Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
933523005:
Push API: Fix thread safety of PushMessagingMessageFilter (Closed)
DescriptionPush API: Fix thread safety of PushMessagingMessageFilter
This addresses several issues:
- Use of weak_factory_io_to_io_ was unsafe because PushMessagingMessageFilter
could be destroyed on either IO or UI thread. Fixed by overriding OnDestruct
to ensure it is always destroyed on IO thread.
- Use of weak_factory_ui_to_ui_ was similarly unsafe. Fixed by splitting out
the UI thread parts of PushMessagingMessageFilter into a new class,
PushMessagingMessageFilterCore, which does all the UI thread work, and is
destoyed on the UI thread. To preserve code clarity, the methods of both
PushMessagingMessageFilter and PushMessagingMessageFilterCore remain
interleaved in the .cc file, so that each of the flows triggered by incoming
messages can be easily traced by reading the methods in order.
- PushMessagingMessageFilter::service() was unsafe because it may be called
after the Profile is destroyed, in which case the cached PushMessagingService
would also have been destroyed. Fixed by removing caching.
BUG=459231
Committed: https://crrev.com/3589edc60b2bf8fcd59abafa3a622bce05ecc368
Cr-Commit-Position: refs/heads/master@{#316897}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Rebase #Patch Set 3 : Add friend class BrowserThread/DeleteHelper #Patch Set 4 : Addressed bauerb's review comments #
Total comments: 6
Patch Set 5 : Address bauerb's 2nd round review comments #Patch Set 6 : Rebase #Patch Set 7 : Fix tests in incognito #
Messages
Total messages: 21 (7 generated)
johnme@chromium.org changed reviewers: + bauerb@chromium.org
Very nice! https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:44: struct RegisterData { (See comment in the header first, because Rietveld sends these in the wrong order) This can also be moved back into the class if Core is a private member (you can still only declare the struct in the header, then define it here). https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:67: friend class PushMessagingMessageFilter; If this class isn't publicly visible anyway, just make the methods called from the PushMessagingMessageFilter public. https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:156: DCHECK_CURRENTLY_ON(BrowserThread::IO); Is this really true? I thought this class is created by the RenderProcessHost, which lives on the UI thread. https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:399: // Only called from IO thread, but would be safe to call from UI thread. I would probably still DCHECK that you're on the IO thread, just to make sure your assumption is correct. https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:589: base::Bind(&PushMessagingMessageFilter::DidUnregister, Instead of passing this as a callback, I think you could directly call this from DidClearRegistrationData(). https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:685: scoped_ptr<IPC::Message> message( You could probably extract this into a Send() method. If you wanted to, you could have that method take a raw pointer, to make it identical to the IPC::Sender version. Or take a scoped_ptr to modernize it. Up to you :) https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:706: if (!service_worker_registration) If you have just DCHECK'ed |service_worker_registration|, you are saying it can't be null (in the absence of bugs), so there is no point in checking it here. Either fail completely silently, or DCHECK and continue (see http://dev.chromium.org/developers/coding-style/#TOC-CHECK-DCHECK-and-NOTREAC...). https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:746: void PushMessagingMessageFilter::ClearRegistrationData( You could move these up to where they are used, to keep the control flow clear? https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:781: : NULL; nullptr plz :) https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... File content/browser/push_messaging/push_messaging_message_filter.h (right): https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.h:34: friend class PushMessagingMessageFilterCore; I would make this an inner class, so you don't have to expose it outside of this class. https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.h:147: scoped_refptr<PushMessagingMessageFilterCore> ui_pmmf_; Just call it |core_|? :)
Another thing I just realized: The PushMessagingMessageFilter is now the only thing that keeps a reference to the core. So, if you wanted to, you could even keep the core as a raw pointer and just post a task to delete it on the UI thread when the filter is destroyed. It means there is more manual memory management, but you don't have the possible implicit side effects from using refcounting. Again, up to you :)
Addressed bauerb@'s review comments - thanks! https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:44: struct RegisterData { On 2015/02/17 16:40:38, Bernhard Bauer wrote: > (See comment in the header first, because Rietveld sends these in the wrong > order) > > This can also be moved back into the class if Core is a private member (you can > still only declare the struct in the header, then define it here). Done. https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:67: friend class PushMessagingMessageFilter; On 2015/02/17 16:40:38, Bernhard Bauer wrote: > If this class isn't publicly visible anyway, just make the methods called from > the PushMessagingMessageFilter public. Done (I'd avoided this because it further fragments the method declarations, but it seems reasonable). https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:156: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2015/02/17 16:40:38, Bernhard Bauer wrote: > Is this really true? I thought this class is created by the RenderProcessHost, > which lives on the UI thread. Right you are! Sorry, serve me right for uploading before I finish rebasing and getting tests passing. I've added a comment explaining the use of weak_factory_io_to_io_.GetWeakPtr() then. https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:399: // Only called from IO thread, but would be safe to call from UI thread. On 2015/02/17 16:40:38, Bernhard Bauer wrote: > I would probably still DCHECK that you're on the IO thread, just to make sure > your assumption is correct. Done (and elsewhere). https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:589: base::Bind(&PushMessagingMessageFilter::DidUnregister, On 2015/02/17 16:40:38, Bernhard Bauer wrote: > Instead of passing this as a callback, I think you could directly call this from > DidClearRegistrationData(). Done. https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:685: scoped_ptr<IPC::Message> message( On 2015/02/17 16:40:38, Bernhard Bauer wrote: > You could probably extract this into a Send() method. If you wanted to, you > could have that method take a raw pointer, to make it identical to the > IPC::Sender version. Or take a scoped_ptr to modernize it. Up to you :) Done (yep, neater that way). https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:706: if (!service_worker_registration) On 2015/02/17 16:40:38, Bernhard Bauer wrote: > If you have just DCHECK'ed |service_worker_registration|, you are saying it > can't be null (in the absence of bugs), so there is no point in checking it > here. Either fail completely silently, or DCHECK and continue (see > http://dev.chromium.org/developers/coding-style/#TOC-CHECK-DCHECK-and-NOTREAC...). Done (good point; this falls under the perhaps not in exceptional cases, so I've removed the DCHECKs for this and the other uses of GetLiveRegistration). https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:746: void PushMessagingMessageFilter::ClearRegistrationData( On 2015/02/17 16:40:38, Bernhard Bauer wrote: > You could move these up to where they are used, to keep the control flow clear? Done (Mounir originally did it this way because he thought we'd end up reusing them, but it seems unlikely now that we will). https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.cc:781: : NULL; On 2015/02/17 16:40:38, Bernhard Bauer wrote: > nullptr plz :) Done. https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... File content/browser/push_messaging/push_messaging_message_filter.h (right): https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.h:34: friend class PushMessagingMessageFilterCore; On 2015/02/17 16:40:38, Bernhard Bauer wrote: > I would make this an inner class, so you don't have to expose it outside of this > class. Done (PushMessagingMessageFilter::Core). https://codereview.chromium.org/933523005/diff/1/content/browser/push_messagi... content/browser/push_messaging/push_messaging_message_filter.h:147: scoped_refptr<PushMessagingMessageFilterCore> ui_pmmf_; On 2015/02/17 16:40:38, Bernhard Bauer wrote: > Just call it |core_|? :) Done (ui_core_, and the reverse is io_parent_).
https://codereview.chromium.org/933523005/diff/60001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/933523005/diff/60001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:169: bool PushMessagingMessageFilter::OnMessageReceived(const IPC::Message& message) Break before the parameter declaration, so you don't have to put the opening brace on the next line? https://codereview.chromium.org/933523005/diff/60001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:244: CheckForExistingRegistration(data, "" /* sender_id */); Nit: use an empty std::string() constructor to save a couple of instructions. https://codereview.chromium.org/933523005/diff/60001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:666: GURL push_endpoint(service()->GetPushEndpoint()); I think you need to check service() for null here, and in two other places (all the call sites for GetPushEndpoint). If the push endpoint is really constant though, you could fetch it at construction time and store it in the filter, which would also save you some thread hopping just to get a constant value.
Addressed bauerb@'s 2nd round review comments On 2015/02/17 17:01:24, Bernhard Bauer wrote: > Another thing I just realized: The PushMessagingMessageFilter is now the only > thing that keeps a reference to the core. So, if you wanted to, you could even > keep the core as a raw pointer and just post a task to delete it on the UI > thread when the filter is destroyed. It means there is more manual memory > management, but you don't have the possible implicit side effects from using > refcounting. Again, up to you :) Done (using scoped_ptr<Core, BrowserThread::DeleteOnUIThread>) https://codereview.chromium.org/933523005/diff/60001/content/browser/push_mes... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/933523005/diff/60001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:169: bool PushMessagingMessageFilter::OnMessageReceived(const IPC::Message& message) On 2015/02/18 10:27:29, Bernhard Bauer wrote: > Break before the parameter declaration, so you don't have to put the opening > brace on the next line? Done. https://codereview.chromium.org/933523005/diff/60001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:244: CheckForExistingRegistration(data, "" /* sender_id */); On 2015/02/18 10:27:29, Bernhard Bauer wrote: > Nit: use an empty std::string() constructor to save a couple of instructions. Done. https://codereview.chromium.org/933523005/diff/60001/content/browser/push_mes... content/browser/push_messaging/push_messaging_message_filter.cc:666: GURL push_endpoint(service()->GetPushEndpoint()); On 2015/02/18 10:27:29, Bernhard Bauer wrote: > I think you need to check service() for null here, and in two other places (all > the call sites for GetPushEndpoint). > > If the push endpoint is really constant though, you could fetch it at > construction time and store it in the filter, which would also save you some > thread hopping just to get a constant value. Done (much neater!)
LGTM, thanks!
New patchsets have been uploaded after l-g-t-m from bauerb@chromium.org
Rebase
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/933523005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by peter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/933523005/100001
The CQ bit was unchecked by johnme@chromium.org
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/933523005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3589edc60b2bf8fcd59abafa3a622bce05ecc368 Cr-Commit-Position: refs/heads/master@{#316897} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
