Fix thread-safety of PushMessagingMessageFilter

Fix thread-safety of PushMessagingMessageFilter

It used weak pointers for passing a `this` reference to callbacks that
get run on different threads, but weak pointers aren't thread-safe in
that way.

Since PushMessagingMessagingFilter extends from BrowserMessageFilter
which is RefCountedThreadSafe, this patch makes it just pass a
scoped_refptr instead (the default behavior of base::Bind).

BUG=438316
Committed: https://crrev.com/0bfd983087bfb4c4a982797acbbbb87a914c5e73
Cr-Commit-Position: refs/heads/master@{#306620}

[johnme, 2014-12-02 19:58:22]

johnme@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2014-12-03 12:21:53]

lgtm, with a question.

https://codereview.chromium.org/770423003/diff/1/content/browser/push_messagi...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/770423003/diff/1/content/browser/push_messagi...
content/browser/push_messaging/push_messaging_message_filter.cc:177: this,
render_frame_id, request_id));
Is there a situation in which the PushMessagingService may not be able to
fulfill a request, e.g. when we send a registration (subscribe) intent to GCM,
but for whatever reason never receive a response?

The weakptr semantics may be useful in the case where we send off a request, but
may not receive a response before the renderer process goes away. Keeping the
PushMessagingMessageFilter alive in that case doesn't have much value, as we're
not going to be able to do anything with it anyway.

The real solution to that problem would obviously be to cancel any in-progress
registrations when the class becomes unnecessary, but I imagine that may be
tricky.

[johnme, 2014-12-03 15:13:53]

Addressed Peter's comment.

https://codereview.chromium.org/770423003/diff/1/content/browser/push_messagi...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/770423003/diff/1/content/browser/push_messagi...
content/browser/push_messaging/push_messaging_message_filter.cc:177: this,
render_frame_id, request_id));
On 2014/12/03 12:21:53, Peter Beverloo wrote:
> The weakptr semantics may be useful in the case where we send off a request,
but
> may not receive a response before the renderer process goes away. Keeping the
> PushMessagingMessageFilter alive in that case doesn't have much value, as
we're
> not going to be able to do anything with it anyway.

That's reasonable; since this is the one case where we're passing from the UI
thread to the UI thread, it's actually safe to use weakptrs there, so I've
re-added them but for this use case only.

[Peter Beverloo, 2014-12-03 15:17:34]

https://codereview.chromium.org/770423003/diff/20001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.h (right):

https://codereview.chromium.org/770423003/diff/20001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.h:92:
base::WeakPtrFactory<PushMessagingMessageFilter> weak_factory_ui_to_ui_;
I'm not a fan of this rename. While it makes it somewhat more explicit, the
comment + thorough reviews should suffice.

Instead, can we revert the rename, and rephrase the comment slightly to
emphasize *why* this is the case?

  // Should only be used for asynchronous calls to the PushMessagingService,
  // which may have external dependencies that supersede the lifetime of this
  // messaging filter.

[johnme, 2014-12-03 15:30:48]

https://codereview.chromium.org/770423003/diff/20001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.h (right):

https://codereview.chromium.org/770423003/diff/20001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.h:92:
base::WeakPtrFactory<PushMessagingMessageFilter> weak_factory_ui_to_ui_;
On 2014/12/03 15:17:34, Peter Beverloo wrote:
> I'm not a fan of this rename. While it makes it somewhat more explicit, the
> comment + thorough reviews should suffice.
> 
> Instead, can we revert the rename, and rephrase the comment slightly to
> emphasize *why* this is the case?
> 
>   // Should only be used for asynchronous calls to the PushMessagingService,
>   // which may have external dependencies that supersede the lifetime of this
>   // messaging filter.

The rename avoids people copy/pasting GetWeakPtr calls for different types of
callback. I adopted this comment though (adding a mention of UI thread only).
Thanks.

[johnme, 2014-12-03 15:31:07]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-12-03 15:31:51]

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

[Michael van Ouwerkerk, 2014-12-03 15:35:19]

mvanouwerkerk@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2014-12-03 15:35:20]

https://codereview.chromium.org/770423003/diff/20001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.h (right):

https://codereview.chromium.org/770423003/diff/20001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.h:92:
base::WeakPtrFactory<PushMessagingMessageFilter> weak_factory_ui_to_ui_;
On 2014/12/03 15:30:48, johnme wrote:
> On 2014/12/03 15:17:34, Peter Beverloo wrote:
> > I'm not a fan of this rename. While it makes it somewhat more explicit, the
> > comment + thorough reviews should suffice.
> > 
> > Instead, can we revert the rename, and rephrase the comment slightly to
> > emphasize *why* this is the case?
> > 
> >   // Should only be used for asynchronous calls to the PushMessagingService,
> >   // which may have external dependencies that supersede the lifetime of
this
> >   // messaging filter.
> 
> The rename avoids people copy/pasting GetWeakPtr calls for different types of
> callback. I adopted this comment though (adding a mention of UI thread only).
> Thanks.

How about just weak_factory_ui_ then?

[commit-bot: I haz the power, 2014-12-03 16:24:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-03 16:25:43]

Patchset 3 (id:??) landed as
https://crrev.com/0bfd983087bfb4c4a982797acbbbb87a914c5e73
Cr-Commit-Position: refs/heads/master@{#306620}