Fix the multi-threaded access to WeakPtr<BrowsingDataRemoverImpl>

Fix the multi-threaded access to WeakPtr<BrowsingDataRemoverImpl>

Previously, we depended on a WeakPtr to access BrowsingDataRemoverImpl
to gain access to its delegate on which we called the embedder origin
type matching method.

With this CL, the delegate provides a callback to a static method,
eliminating the need for any pointers.

To prevent a similar bug from happening in the future, we proactively
bind the WeakPtr<BrowsingDataRemoverImpl> to the UI thread, so it
would immediately DCHECK if it was misused on the IO thread.

BUG=705538, 668114
Review-Url: https://codereview.chromium.org/2781083002
Cr-Commit-Position: refs/heads/master@{#462530}
Committed: https://chromium.googlesource.com/chromium/src/+/c9f101b745929a50b391abf446cefb6b2f4b4bd6

[msramek, 2017-03-29 13:47:12]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 13:47:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-03-29 13:50:32]

Description was changed from

==========
Fix the multi-threaded access to WeakPtr<BrowsingDataRemoverImpl>

The scenario that caused crbug.com/705338 is the following:
1. BrowsingDataRemoverImpl lives on the UI thread.
2. It calls a method in the StoragePartition on the UI thread, passing
   it a callback with a WeakPtr<BrowsingDataRemoverImpl>.
3. Storage partition jumps to the IO thread and uses the WeakPtr there.
4. DCHECK, because as per #1 the WeakPtr lives on the UI thread.

Solution:
1. Extract the method that is passed in the callback to a separate class.
2. Hold the class by ref counting from both BrowsingDataRemoverImpl
   and StoragePartition. When BrowsingDataRemover is destroyed, the
   helper class is destroyed as well unless the StoragePartition is still
   using it (i.e. while the callback lives).
3. The callback does not depend on any state from BrowsingDataRemoverImpl
   except the |embedder_delegate_| link. And the only thing from
   BrowsingDataRemoverDelegate that is needed is one static method.
   method. Therefore the BrowsingDataRemoverEmbedderDelegate interface
   has been changed to provide that static method so it can be injected
   into our helper class.

BUG=705538
==========

to

==========
Fix the multi-threaded access to WeakPtr<BrowsingDataRemoverImpl>

The scenario that caused crbug.com/705338 is the following:
1. BrowsingDataRemoverImpl lives on the UI thread.
2. It calls a method in the StoragePartition on the UI thread, passing
   it a callback with a WeakPtr<BrowsingDataRemoverImpl>.
3. Storage partition jumps to the IO thread and uses the WeakPtr there.
4. DCHECK, because as per #1 the WeakPtr lives on the UI thread.

Solution:
1. Extract the method that is passed in the callback to a separate class.
2. Hold the class by ref counting from both BrowsingDataRemoverImpl
   and StoragePartition. When BrowsingDataRemover is destroyed, the
   helper class is destroyed as well unless the StoragePartition is still
   using it (i.e. while the callback lives).
3. The callback does not depend on any state from BrowsingDataRemoverImpl
   except the |embedder_delegate_| link. And the only thing from
   BrowsingDataRemoverDelegate that is needed is one static method.
   method. Therefore the BrowsingDataRemoverEmbedderDelegate interface
   has been changed to provide that static method so it can be injected
   into our helper class.

BUG=705538,668114
==========

[commit-bot: I haz the power, 2017-03-29 14:21:39]

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

[commit-bot: I haz the power, 2017-03-29 14:21:40]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[msramek, 2017-03-29 15:59:09]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 16:00:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 16:46:39]

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

[commit-bot: I haz the power, 2017-03-29 16:46:40]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-03-29 16:47:46]

Patchset #1 (id:1) has been deleted

[msramek, 2017-03-29 16:54:09]

msramek@chromium.org changed reviewers:
+ bauerb@chromium.org

[msramek, 2017-03-29 16:54:10]

Hi Bernhard,

Can you have a look? I'm sending this to you because you reviewed the CL where I
introduced the bug *and* you also reported it :-)

Regarding your comment from the issue tracker
(https://bugs.chromium.org/p/chromium/issues/detail?id=705973#c2), I'm not sure
it applies - the WeakPtrFactory indeed was created and destroyed on the UI
thread the whole time, it's just that the WeakPtr itself was used on the IO
thread.

Anyways, the CL description explains what happened. While my solution feels
quite overengineered to me, I'm not aware of any multi-threaded WeakPtr
solutions that could be applied more straightforwardly.

It would also have helped if C++ supported class methods (i.e. methods that vary
over subclass but *not* instance), then we would get rid of the one problematic
pointer. But well, it doesn't.

Thanks!
Martin

[Bernhard Bauer, 2017-03-30 10:49:26]

On 2017/03/29 16:54:10, msramek wrote:
> Regarding your comment from the issue tracker
> (https://bugs.chromium.org/p/chromium/issues/detail?id=705973#c2), I'm not
sure
> it applies - the WeakPtrFactory indeed was created and destroyed on the UI
> thread the whole time, it's just that the WeakPtr itself was used on the IO
> thread.

Here's the thing though -- the stack trace with the DCHECK actually is on the UI
thread! :) I think this is due to the fact that the flag used by the weak
pointer only becomes bound when checking for validity or invalidating it, not on
construction (see
https://cs.chromium.org/chromium/src/base/memory/weak_ptr.cc?type=cs&sq=packa...),
so the WeakPtr becomes bound the first time it's dereferenced, and that happens
to be on the IO thread.

https://codereview.chromium.org/2781083002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_delegate.h (right):

https://codereview.chromium.org/2781083002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_delegate.h:38: // to handle
ALL embedder-defined typed. It must be static and support
Nit: The callback doesn't have to be static, it's just that that is an easy way
to ensure thread safety.

https://codereview.chromium.org/2781083002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_impl.h (right):

https://codereview.chromium.org/2781083002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_impl.h:208:
MaskMatcherFunction embedder_matcher_;
If this is the only state in the class, and it's a callback, so the object
itself is thread-safe, why do we need to wrap it in a class? Couldn't we just
make DoesOriginMatchMaskAndURLs() static and bind this as an additional
parameter?

[msramek, 2017-04-06 11:52:06]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 11:52:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-06 12:00:49]

Description was changed from

==========
Fix the multi-threaded access to WeakPtr<BrowsingDataRemoverImpl>

The scenario that caused crbug.com/705338 is the following:
1. BrowsingDataRemoverImpl lives on the UI thread.
2. It calls a method in the StoragePartition on the UI thread, passing
   it a callback with a WeakPtr<BrowsingDataRemoverImpl>.
3. Storage partition jumps to the IO thread and uses the WeakPtr there.
4. DCHECK, because as per #1 the WeakPtr lives on the UI thread.

Solution:
1. Extract the method that is passed in the callback to a separate class.
2. Hold the class by ref counting from both BrowsingDataRemoverImpl
   and StoragePartition. When BrowsingDataRemover is destroyed, the
   helper class is destroyed as well unless the StoragePartition is still
   using it (i.e. while the callback lives).
3. The callback does not depend on any state from BrowsingDataRemoverImpl
   except the |embedder_delegate_| link. And the only thing from
   BrowsingDataRemoverDelegate that is needed is one static method.
   method. Therefore the BrowsingDataRemoverEmbedderDelegate interface
   has been changed to provide that static method so it can be injected
   into our helper class.

BUG=705538,668114
==========

to

==========
Fix the multi-threaded access to WeakPtr<BrowsingDataRemoverImpl>

Previously, we depended on a WeakPtr to access BrowsingDataRemoverImpl
to gain access to its delegate on which we called the embedder origin
type matching method.

With this CL, the delegate provides a callback to a static method,
eliminating the need for any pointers.

To prevent a similar bug from happening in the future, we proactively
bind the WeakPtr<BrowsingDataRemoverImpl> to the UI thread, so it
would immediately DCHECK if it was misused on the IO thread.

BUG=705538,668114
==========

[msramek, 2017-04-06 12:06:39]

Sorry for the delay / context loss, I haven't had time to return to this
earlier. And thanks a lot for the advice! It's much more simple and
understandable now :)

https://codereview.chromium.org/2781083002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_delegate.h (right):

https://codereview.chromium.org/2781083002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_delegate.h:38: // to handle
ALL embedder-defined typed. It must be static and support
On 2017/03/30 10:49:26, Bernhard (slow until Apr 6) wrote:
> Nit: The callback doesn't have to be static, it's just that that is an easy
way
> to ensure thread safety.

Indeed. I originally wrote that it "must outlive the associated
BrowsingDataRemover", but then I noticed that there is in fact no reference to
BrowsingDataRemover in this interface, so that might be confusing. I think it's
less confusing to have a stronger requirement.

https://codereview.chromium.org/2781083002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_impl.h (right):

https://codereview.chromium.org/2781083002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_impl.h:208:
MaskMatcherFunction embedder_matcher_;
On 2017/03/30 10:49:26, Bernhard (slow until Apr 6) wrote:
> If this is the only state in the class, and it's a callback, so the object
> itself is thread-safe, why do we need to wrap it in a class? Couldn't we just
> make DoesOriginMatchMaskAndURLs() static and bind this as an additional
> parameter?

Because I panicked and overengineered it. Done.

https://codereview.chromium.org/2781083002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_delegate.h (right):

https://codereview.chromium.org/2781083002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_delegate.h:31:
EmbedderOriginTypeMatcher;
I renamed this and the getter. Since we removed the helper class, there is no
need to distinguish that this is a "Function", and that is a class; instead, I
emphasized what it's for (EmbedderOriginType).

https://codereview.chromium.org/2781083002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_impl.cc (right):

https://codereview.chromium.org/2781083002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_impl.cc:686: weak_ptr.get();
I added the proactive UI thread binding here.

[msramek, 2017-04-06 12:09:15]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 12:09:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 12:57:27]

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

[commit-bot: I haz the power, 2017-04-06 12:57:28]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-04-06 15:02:13]

LGTM, thanks!

https://codereview.chromium.org/2781083002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h
(right):

https://codereview.chromium.org/2781083002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h:199: static
bool DoesOriginMatchEmbedderMask(
Does this need to be part of the class, or could it live in an anonymous
namespace in the .cc file? I don't see anything in there that requires private
access to class members. (If there is something, please add a comment here :) )

[msramek, 2017-04-06 16:25:53]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[msramek, 2017-04-06 16:27:31]

Thank you!

https://codereview.chromium.org/2781083002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h
(right):

https://codereview.chromium.org/2781083002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h:199: static
bool DoesOriginMatchEmbedderMask(
On 2017/04/06 15:02:12, Bernhard Bauer wrote:
> Does this need to be part of the class, or could it live in an anonymous
> namespace in the .cc file? I don't see anything in there that requires private
> access to class members. (If there is something, please add a comment here :)
)

Done. No, it doesn't.

[msramek, 2017-04-06 16:27:35]

The CQ bit was unchecked by msramek@chromium.org

[msramek, 2017-04-06 16:27:39]

The CQ bit was checked by msramek@chromium.org

[msramek, 2017-04-06 16:27:39]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2781083002/#ps80001
(title: "Static method to local namespace.")

[commit-bot: I haz the power, 2017-04-06 16:33:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 17:29:57]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491496059273980,
"parent_rev": "76fcce24671c86169992b4e4f120bb1498f7c7fd", "commit_rev":
"c9f101b745929a50b391abf446cefb6b2f4b4bd6"}

[commit-bot: I haz the power, 2017-04-06 17:30:50]

Description was changed from

==========
Fix the multi-threaded access to WeakPtr<BrowsingDataRemoverImpl>

Previously, we depended on a WeakPtr to access BrowsingDataRemoverImpl
to gain access to its delegate on which we called the embedder origin
type matching method.

With this CL, the delegate provides a callback to a static method,
eliminating the need for any pointers.

To prevent a similar bug from happening in the future, we proactively
bind the WeakPtr<BrowsingDataRemoverImpl> to the UI thread, so it
would immediately DCHECK if it was misused on the IO thread.

BUG=705538,668114
==========

to

==========
Fix the multi-threaded access to WeakPtr<BrowsingDataRemoverImpl>

Previously, we depended on a WeakPtr to access BrowsingDataRemoverImpl
to gain access to its delegate on which we called the embedder origin
type matching method.

With this CL, the delegate provides a callback to a static method,
eliminating the need for any pointers.

To prevent a similar bug from happening in the future, we proactively
bind the WeakPtr<BrowsingDataRemoverImpl> to the UI thread, so it
would immediately DCHECK if it was misused on the IO thread.

BUG=705538,668114

Review-Url: https://codereview.chromium.org/2781083002
Cr-Commit-Position: refs/heads/master@{#462530}
Committed:
https://chromium.googlesource.com/chromium/src/+/c9f101b745929a50b391abf446ce...
==========

[commit-bot: I haz the power, 2017-04-06 17:30:51]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c9f101b745929a50b391abf446ce...