Remove all but one use of WeakPtrFactory::DetachFromThread.

Remove all but one use of WeakPtrFactory::DetachFromThread.

This CL changes WeakPtr in the following ways:
* Changes thread-bindings semantics so that WeakPtrs only become bound when the first one is dereferenced, or the owning factory invalidates them.
* Removes WeakPtrFactory::DetachFromThread.
* Renames SupportsWeakPtr::DetachFromThread  to DetachFromThreadHack.

Calling code changes to allow this:
* Unnecessary DetachFromThread() calls removed from PluginInfoMessageFilter, DhcpProxyScript[Adapter]FetcherWin and (Chromoting's) PolicyWatcherLinux.
* DetachFromThread() calls rendered unnecessary by change in binding semantics removed from IOThread, SearchProviderInstallData, RuleRegistryWithCache and GLSurfaceGlx.

WebGraphicsContext3DInProcessCommandBufferImpl uses the re-named DetachFromThreadHack() - bug 234964 tracks work to remove that use.

BUG=232143, 234964
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202038

[Wez, 2013-04-17 02:22:31]

PTAL

It looks like DetachFromThread can disappear except for one non-trivial use in
WebGraphicsContext3D, which I'm looking into.

However, it does require a change to have WeakPtr[Factory] late-bind to a thread
only when dereferenced or invalidated, rather than when the first pointer is
taken, to allow an object to create a WeakPtr when created on one thread, but
the run, and tear-down, on a second thread.

Let me know what you think.

[Peter Kasting, 2013-04-17 02:27:39]

On 2013/04/17 02:22:31, Wez wrote:
> However, it does require a change to have WeakPtr[Factory] late-bind to a
thread
> only when dereferenced or invalidated, rather than when the first pointer is
> taken, to allow an object to create a WeakPtr when created on one thread, but
> the run, and tear-down, on a second thread.

Is it possible to make these classes live entirely on one thread?

[Wez, 2013-04-17 02:39:15]

On 2013/04/17 02:27:39, Peter Kasting wrote:
> On 2013/04/17 02:22:31, Wez wrote:
> > However, it does require a change to have WeakPtr[Factory] late-bind to a
> thread
> > only when dereferenced or invalidated, rather than when the first pointer is
> > taken, to allow an object to create a WeakPtr when created on one thread,
but
> > the run, and tear-down, on a second thread.
> 
> Is it possible to make these classes live entirely on one thread?

See ui/gl/gl_surface_gl.cc; the weak-referenced class lives on vsync_thread_,
except that it is created on the owning object's thread.  Rearranging that to
also create the object on the other thread would be a pain.

[Wez, 2013-05-10 00:40:41]

I've updated this CL to land pending removal of the DetachFromThread call from
the 3D code, and renamed the API to hopefully make it clearer that it shouldn't
be used in new code.

PTAL

[Wez, 2013-05-15 00:59:50]

Albert, PTAL!

[awong, 2013-05-15 01:19:15]

https://codereview.chromium.org/14299011/diff/31016/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/14299011/diff/31016/base/memory/weak_ptr.h#ne...
base/memory/weak_ptr.h:22: //    // Member variables should appear before the
WeakPtrFactory.
Explain why.

https://codereview.chromium.org/14299011/diff/31016/base/memory/weak_ptr.h#ne...
base/memory/weak_ptr.h:55: // pointers for may be moved to a new thread.
Would it be useful to list a reduction of the following snippet in the bug:

1. When a[n object containing a] WeakPtrFactory is created, it is un-bound.
2. Dispensing a WeakPtr from WeakPtrFactory does not cause it to become bound.
3. When a WeakPtr is dereferenced, it and the creating WeakPtrFactory become
bound.
4. When a WeakPtrFactory is invalidated, issued WeakPtrs become bound, and the
WeakPtrFactory itself becomes un-bound.

Or would it be too much info?

[Wez, 2013-05-15 22:51:50]

https://chromiumcodereview.appspot.com/14299011/diff/31016/base/memory/weak_p...
File base/memory/weak_ptr.h (right):

https://chromiumcodereview.appspot.com/14299011/diff/31016/base/memory/weak_p...
base/memory/weak_ptr.h:22: //    // Member variables should appear before the
WeakPtrFactory.
On 2013/05/15 01:19:15, awong wrote:
> Explain why.

Done.

https://chromiumcodereview.appspot.com/14299011/diff/31016/base/memory/weak_p...
base/memory/weak_ptr.h:55: // pointers for may be moved to a new thread.
On 2013/05/15 01:19:15, awong wrote:
> Would it be useful to list a reduction of the following snippet in the bug:
> 
> 1. When a[n object containing a] WeakPtrFactory is created, it is un-bound.
> 2. Dispensing a WeakPtr from WeakPtrFactory does not cause it to become bound.
> 3. When a WeakPtr is dereferenced, it and the creating WeakPtrFactory become
> bound.
> 4. When a WeakPtrFactory is invalidated, issued WeakPtrs become bound, and the
> WeakPtrFactory itself becomes un-bound.
> 
> Or would it be too much info?

I've added that in.  It feels to me a little too implementation-detail oriented
- I'm thinking of a cleaner model for expressing this.

[Wez, 2013-05-17 04:47:53]

PTAL - have tried to simplify the threading comments.

[awong, 2013-05-17 19:02:31]

One more comment but looking good.

https://codereview.chromium.org/14299011/diff/52001/base/memory/weak_ptr_unit...
File base/memory/weak_ptr_unittest.cc (right):

https://codereview.chromium.org/14299011/diff/52001/base/memory/weak_ptr_unit...
base/memory/weak_ptr_unittest.cc:541: ASSERT_DEATH(delete foo, "");
Should this death test bother matching for the log message?

[Wez, 2013-05-18 07:02:35]

https://codereview.chromium.org/14299011/diff/52001/base/memory/weak_ptr_unit...
File base/memory/weak_ptr_unittest.cc (right):

https://codereview.chromium.org/14299011/diff/52001/base/memory/weak_ptr_unit...
base/memory/weak_ptr_unittest.cc:541: ASSERT_DEATH(delete foo, "");
On 2013/05/17 19:02:32, awong wrote:
> Should this death test bother matching for the log message?

Done.

[Wez, 2013-05-20 02:13:36]

darin, could you provide the uber-LGTM for this, please?

[awong, 2013-05-21 21:53:47]

LGTM

Looks like the death tests might be causing an issue on IOS. I'd prefer to have
the regex in otherwise the test will pass on any sort of death (related or not)
but if it's too much trouble, happy to let it go.

[darin (slow to review), 2013-05-23 22:27:46]

LGTM

[Wez, 2013-05-24 06:21:27]

On 2013/05/21 21:53:47, awong wrote:
> LGTM
> 
> Looks like the death tests might be causing an issue on IOS. I'd prefer to
have
> the regex in otherwise the test will pass on any sort of death (related or
not)
> but if it's too much trouble, happy to let it go.

I can't find any other ASSERT_DEATH() clauses in base/ that I'd expect to be
built on iOS that specify a regex, so I'm going to land without the regex and
file a bug about that failing on iOS.

[commit-bot: I haz the power, 2013-05-24 06:21:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/14299011/71001

[commit-bot: I haz the power, 2013-05-24 09:51:38]

Change committed as 202038