Fix ThreadChecker bug in WeakNSObject

Fix ThreadChecker bug in WeakNSObject

When a WeakNSObject was copied on a different thread from where it was
created, the state of the thread checker was lost.
As a consequence it was not possible to send a WeakNSObject on another
thread, copy it there, and send the copy back: a ThreadChecker error
was raised.

This CL adds more thread checks in the WeakContainer, which matches
what WeakPtr also does, and keeps most of the checks in the main
class (which is stricter than WeakPtr).

Committed: https://crrev.com/4f05f5a8b793ec451683dd96c058cbe74037713d
Cr-Commit-Position: refs/heads/master@{#311865}

[droger, 2015-01-14 09:31:52]

droger@chromium.org changed reviewers:
+ lliabra@chromium.org

[droger, 2015-01-14 09:31:52]

I tried that fix downstream and it passed the trybots:
https://chromereviews.googleplex.com/134567013/

[droger, 2015-01-14 10:48:36]

droger@chromium.org changed reviewers:
+ lliabraa@chromium.org, noyau@chromium.org
- lliabra@chromium.org

[droger, 2015-01-14 10:48:37]

+noyau

After discussion with noyau@, it's not completely clear if this change is good
or not.

I'm trying another version (patch set 2) that restores the thread checks that
were removed, but still allows the weak object to be copied on another thread.
It is still running on the downstream bots.

[droger, 2015-01-14 10:48:53]

Patchset #2 (id:20001) has been deleted

[droger, 2015-01-14 13:19:23]

On 2015/01/14 10:48:37, droger wrote:
> I'm trying another version (patch set 2) that restores the thread checks that
> were removed, but still allows the weak object to be copied on another thread.
> It is still running on the downstream bots.

The version with more checks also passes downstream.

[noyau (Ping after 24h), 2015-01-14 14:29:56]

lgtm

[lliabraa, 2015-01-14 14:32:38]

On 2015/01/14 14:29:56, noyau wrote:
> lgtm

@noyau - why is it important for the weakNSObject to have a thread checker
instead of jsut the container?

[noyau (Ping after 24h), 2015-01-14 15:14:57]

On 2015/01/14 14:32:38, lliabraa wrote:
> On 2015/01/14 14:29:56, noyau wrote:
> > lgtm
> 
> @noyau - why is it important for the weakNSObject to have a thread checker
> instead of jsut the container?

This avoids misuse of a weakNSObject on one thread while it is reset() on
another for example. Making a copy to send it to another thread seems more
sensible rather than using the same instance on multiple threads at once

[lliabraa, 2015-01-14 15:25:00]

https://codereview.chromium.org/853503002/diff/40001/base/ios/weak_nsobject.h
File base/ios/weak_nsobject.h (right):

https://codereview.chromium.org/853503002/diff/40001/base/ios/weak_nsobject.h...
base/ios/weak_nsobject.h:40: // same thread that the WeakNSObject is created on.
how about expanding this section on threading with examples of what's allowed
and what's not?

https://codereview.chromium.org/853503002/diff/40001/base/ios/weak_nsobject.h...
base/ios/weak_nsobject.h:87: checker_.DetachFromThread();
won't calling DetachFromThread() here cause the WeakNSObject to not be
associated with any thread when it's created? Seems like we only want to call
DetachFromThread() in the copy constructor.

[droger, 2015-01-14 15:50:04]

I addressed the comments.

[lliabraa, 2015-01-15 13:00:56]

lgtm

[droger, 2015-01-15 13:03:24]

droger@chromium.org changed reviewers:
+ stuartmorgan@chromium.org

[droger, 2015-01-15 13:03:24]

+ Stuart as OWNER

[stuartmorgan, 2015-01-15 14:51:42]

lgtm

https://codereview.chromium.org/853503002/diff/80001/base/ios/weak_nsobject_u...
File base/ios/weak_nsobject_unittest.mm (right):

https://codereview.chromium.org/853503002/diff/80001/base/ios/weak_nsobject_u...
base/ios/weak_nsobject_unittest.mm:100: void TouchWeakData(const
WeakNSObject<NSMutableData>& weak_data) {
Please comment the utility methods.

[droger, 2015-01-15 15:38:52]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2015-01-15 15:39:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/853503002/100001

[commit-bot: I haz the power, 2015-01-15 17:21:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-15 17:21:18]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[droger, 2015-01-15 17:49:24]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2015-01-15 17:49:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/853503002/100001

[commit-bot: I haz the power, 2015-01-15 17:50:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-15 17:50:18]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[droger, 2015-01-16 10:15:47]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2015-01-16 10:16:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/853503002/100001

[commit-bot: I haz the power, 2015-01-16 11:34:21]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-01-16 11:35:53]

Patchset 5 (id:??) landed as
https://crrev.com/4f05f5a8b793ec451683dd96c058cbe74037713d
Cr-Commit-Position: refs/heads/master@{#311865}