Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1575283003:
Move notification click event dispatching out of ServiceWorkerVersion. (Closed)
|
Created:
4 years, 11 months ago by Marijn Kruisselbrink Modified:
4 years, 11 months ago CC:
blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+serviceworker, kinuko+watch, michaeln, mlamouri+watch-notifications_chromium.org, nhiroki, Peter Beverloo, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@swversion-ipc-refactor Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove notification click event dispatching out of ServiceWorkerVersion.
BUG=570820
Committed: https://crrev.com/d9ef31156d4012582b3d551e584666f4524aa493
Cr-Commit-Position: refs/heads/master@{#371559}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Make callback paramters const-ref #
Total comments: 24
Patch Set 3 : nits #
Total comments: 3
Patch Set 4 : rebase #Patch Set 5 : rebase and use DispatchSimpleEvent #
Depends on Patchset: Messages
Total messages: 24 (8 generated)
Description was changed from ========== Move click notification event dispatching out of ServiceWorkerVersion. BUG=570820 ========== to ========== Move notification click event dispatching out of ServiceWorkerVersion. BUG=570820 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
mek@chromium.org changed reviewers: + johnme@chromium.org, nhiroki@chromium.org
johnme: please review nhiroki: the SW changes are purely removing the old code from SWVersion
lgtm https://codereview.chromium.org/1575283003/diff/40001/content/browser/notific... File content/browser/notifications/notification_event_dispatcher_impl.cc (right): https://codereview.chromium.org/1575283003/diff/40001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:77: scoped_refptr<ServiceWorkerVersion> service_worker, nit: const-ref https://codereview.chromium.org/1575283003/diff/40001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:78: base::Callback<void(ServiceWorkerStatusCode)> callback, ditto.
https://codereview.chromium.org/1575283003/diff/40001/content/browser/notific... File content/browser/notifications/notification_event_dispatcher_impl.cc (right): https://codereview.chromium.org/1575283003/diff/40001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:77: scoped_refptr<ServiceWorkerVersion> service_worker, On 2016/01/15 at 00:23:33, nhiroki wrote: > nit: const-ref There was actually a (too) subtle reason why this couldn't be a const-ref (the ref would point at data that would get deleted by the FinishRequest call below), but in trying to explain why this was unavoidable I realized it isn't in fact unavoidable. See the ServiceWorkerVersion::EventResponseHandler::OnMessageReceived changes in service_worker_version.h in the latest patch.
Some drive-by nits. Thanks Marijn :-) https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... File content/browser/notifications/notification_event_dispatcher_impl.cc (right): https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:76: void OnNotificationClickEventReply( nit: add some documentation like the other functions https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:79: int request_id) { nit: DCHECK_CURRENTLY_ON like all other functions in this file https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:87: void DispatchNotificationClickEventOnWorker( nit: add some documentation like the other functions https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:91: const base::Callback<void(ServiceWorkerStatusCode)>& callback) { nit: DCHECK_CURRENTLY_ON like all other functions in this file https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:127: dispatch_event_callback, nit: I would expect the error callback argument to go after the main task argument, like in most code I've seen elsewhere. I appreciate that RunAfterStartWorker is an existing API so it might not be appropriate to change in this CL.
Looks generally good - thanks! https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... File content/browser/notifications/notification_event_dispatcher_impl.cc (right): https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:78: const base::Callback<void(ServiceWorkerStatusCode)>& callback, Nit: may as well use `const ServiceWorkerVersion::StatusCallback&` since this has to match the first parameter of RunAfterStartWorker. https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:83: service_worker->FinishRequest(request_id); Please NOTREACHED() if this returns false. https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:84: callback.Run(SERVICE_WORKER_OK); The previous code did the following: scoped_refptr<ServiceWorkerVersion> protect(this); request->callback.Run(SERVICE_WORKER_OK); RemoveCallbackAndStopIfRedundant(¬ification_click_requests_, request_id); In order to explicitly keep the Service Worker alive whilst the callback was running. But here, you don't have a protect refptr, and instead you call FinishRequest - and hence RemoveCallbackAndStopIfRedundant - before calling the callback. Is this still safe? https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:91: const base::Callback<void(ServiceWorkerStatusCode)>& callback) { Ditto `const ServiceWorkerVersion::StatusCallback&` https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:126: service_worker_registration->active_version()->RunAfterStartWorker( Is it ok to call RunAfterStartWorker (and hence StartWorker) if the SW is already running? ServiceWorkerVersion::DispatchNotificationClickEvent would only call StartWorker if it wasn't already running. For example some new side-effects that I noticed are that StartWorker will overwrite prestart_status_, and DidEnsureLiveRegistrationForStartWorker will call MarkIfStale. https://codereview.chromium.org/1575283003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (left): https://codereview.chromium.org/1575283003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_version.cc:634: DCHECK_EQ(ACTIVATED, status()) << status(); Is there any equivalent to this DCHECK in the new version?
https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... File content/browser/notifications/notification_event_dispatcher_impl.cc (right): https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:76: void OnNotificationClickEventReply( On 2016/01/15 at 16:44:03, Michael van Ouwerkerk wrote: > nit: add some documentation like the other functions Done https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:78: const base::Callback<void(ServiceWorkerStatusCode)>& callback, On 2016/01/15 at 16:52:58, johnme wrote: > Nit: may as well use `const ServiceWorkerVersion::StatusCallback&` since this has to match the first parameter of RunAfterStartWorker. Agreed; not sure why the code in DispatchNotificationClickEventOnRegistration used the explicit base::Callback type rather than just using SWVersion::StatusCallback. Changed it there too for consistency. https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:79: int request_id) { On 2016/01/15 at 16:44:03, Michael van Ouwerkerk wrote: > nit: DCHECK_CURRENTLY_ON like all other functions in this file Done https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:83: service_worker->FinishRequest(request_id); On 2016/01/15 at 16:52:58, johnme wrote: > Please NOTREACHED() if this returns false. Done https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:84: callback.Run(SERVICE_WORKER_OK); On 2016/01/15 at 16:52:58, johnme wrote: > The previous code did the following: > > scoped_refptr<ServiceWorkerVersion> protect(this); > request->callback.Run(SERVICE_WORKER_OK); > RemoveCallbackAndStopIfRedundant(¬ification_click_requests_, request_id); > > In order to explicitly keep the Service Worker alive whilst the callback was running. But here, you don't have a protect refptr, and instead you call FinishRequest - and hence RemoveCallbackAndStopIfRedundant - before calling the callback. > > Is this still safe? If I understand it correctly, the old code made sure to keep the SWVersion alive not so much for the benefit of the callback, but to make sure the subsequent RemoveCallbackAndStopIfRedundant call could still proceed and |this| would still be valid at that point (and maybe as well to make sure the actual callback wouldn't get deleted while it was running). So I believe the current code is just as safe: to be able to call FinishRequest code must have a valid ServiceWorkerVersion pointer, and nothing FinishRequest does could cause the SWVersion to actually get deleted. Also the actual method here where FinishRequest is called from has a scoped_refptr to the ServiceWorkerVersion, so even if that's the only reference left it would still survive past calling the callback. https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:87: void DispatchNotificationClickEventOnWorker( On 2016/01/15 at 16:44:03, Michael van Ouwerkerk wrote: > nit: add some documentation like the other functions Done https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:91: const base::Callback<void(ServiceWorkerStatusCode)>& callback) { On 2016/01/15 at 16:52:58, johnme wrote: > Ditto `const ServiceWorkerVersion::StatusCallback&` Done https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:126: service_worker_registration->active_version()->RunAfterStartWorker( On 2016/01/15 at 16:52:58, johnme wrote: > Is it ok to call RunAfterStartWorker (and hence StartWorker) if the SW is already running? ServiceWorkerVersion::DispatchNotificationClickEvent would only call StartWorker if it wasn't already running. > > For example some new side-effects that I noticed are that StartWorker will overwrite prestart_status_, and DidEnsureLiveRegistrationForStartWorker will call MarkIfStale. Good question; I don't think the side effects will be problematic, but no need to actually change behavior for this, so in https://codereview.chromium.org/1584653009 I changed the RunAfterStartWorker implementation to match its description better and only call StartWorker if the worker isn't already running. https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:127: dispatch_event_callback, On 2016/01/15 at 16:44:03, Michael van Ouwerkerk wrote: > nit: I would expect the error callback argument to go after the main task argument, like in most code I've seen elsewhere. I appreciate that RunAfterStartWorker is an existing API so it might not be appropriate to change in this CL. Changed this order in https://codereview.chromium.org/1584653009 as well (and will of course make sure to update whichever CL lands second to include changes from the other). https://codereview.chromium.org/1575283003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (left): https://codereview.chromium.org/1575283003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_version.cc:634: DCHECK_EQ(ACTIVATED, status()) << status(); On 2016/01/15 at 16:52:58, johnme wrote: > Is there any equivalent to this DCHECK in the new version? Good question. Not currently. I considered adding something like DCHECK(status() == ACTIVATED || event_type == INSTALL || event_type == ACTIVATE || event_type == MESSAGE) to StartRequest, but that didn't feel right to me. Also with the changes to ServiceWorkerContextWrapper were it now generally will only return a registration with a fully activated worker (unless you call GetAllRegistrations) it seemed the risk of triggering this DCHECK is much lower than what it was when this code was originally written. But I could easily be convinced that a check like this is still meaningful. I'm not sure if it should be in feature specific code (where it would be checking that FindReadyRegistration* in fact did return a fully activated worker) or if the check should go in StartRequest (where I'd rather not hardcode the list of events that can be send to a worker that isn't active).
lgtm with nits - thanks :) https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... File content/browser/notifications/notification_event_dispatcher_impl.cc (right): https://codereview.chromium.org/1575283003/diff/60001/content/browser/notific... content/browser/notifications/notification_event_dispatcher_impl.cc:84: callback.Run(SERVICE_WORKER_OK); On 2016/01/15 23:48:49, Marijn Kruisselbrink wrote: > On 2016/01/15 at 16:52:58, johnme wrote: > > The previous code did the following: > > > > scoped_refptr<ServiceWorkerVersion> protect(this); > > request->callback.Run(SERVICE_WORKER_OK); > > RemoveCallbackAndStopIfRedundant(¬ification_click_requests_, request_id); > > > > In order to explicitly keep the Service Worker alive whilst the callback was > running. But here, you don't have a protect refptr, and instead you call > FinishRequest - and hence RemoveCallbackAndStopIfRedundant - before calling the > callback. > > > > Is this still safe? > > If I understand it correctly, the old code made sure to keep the SWVersion alive > not so much for the benefit of the callback, but to make sure the subsequent > RemoveCallbackAndStopIfRedundant call could still proceed and |this| would still > be valid at that point (and maybe as well to make sure the actual callback > wouldn't get deleted while it was running). You're right - the (somewhat awkward) pattern is for the callback to be bound with a scoped_refptr to the SW (just to keep it alive - the callback doesn't use it), so once the callback finishes the refcount is decremented and the SW might be destroyed. > So I believe the current code is just as safe: to be able to call FinishRequest > code must have a valid ServiceWorkerVersion pointer, and nothing FinishRequest > does could cause the SWVersion to actually get deleted. Also the actual method > here where FinishRequest is called from has a scoped_refptr to the > ServiceWorkerVersion, so even if that's the only reference left it would still > survive past calling the callback. Indeed, this seems just as safe. I wonder if it might perform slightly worse, in the (currently theoretical) case where the callback synchronously dispatches a new event to the SW? With the new code, it's possible that the SW would be stopped due to idleness before the callback is run, and hence the SW would have to be restarted. But feel free to keep the new order if it has benefits. https://codereview.chromium.org/1575283003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (left): https://codereview.chromium.org/1575283003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_version.cc:634: DCHECK_EQ(ACTIVATED, status()) << status(); On 2016/01/15 23:48:49, Marijn Kruisselbrink wrote: > On 2016/01/15 at 16:52:58, johnme wrote: > > Is there any equivalent to this DCHECK in the new version? > > Good question. Not currently. I considered adding something like DCHECK(status() > == ACTIVATED || event_type == INSTALL || event_type == ACTIVATE || event_type == > MESSAGE) to StartRequest, but that didn't feel right to me. Also with the > changes to ServiceWorkerContextWrapper were it now generally will only return a > registration with a fully activated worker (unless you call GetAllRegistrations) > it seemed the risk of triggering this DCHECK is much lower than what it was when > this code was originally written. > > But I could easily be convinced that a check like this is still meaningful. I'm > not sure if it should be in feature specific code (where it would be checking > that FindReadyRegistration* in fact did return a fully activated worker) or if > the check should go in StartRequest (where I'd rather not hardcode the list of > events that can be send to a worker that isn't active). Both push and notifications have experienced a lot of subtle bugs about Service Workers not being in the right state. I'd rather keep this check in some form. Having a whitelist of events which don't need an activated Service Worker actually sounds quite reasonable, since this should be a small finite list (possibly just INSTALL and ACTIVATE? Why would we dispatch a MESSAGE to an inactive SW?).
One more comment. https://codereview.chromium.org/1575283003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/1575283003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_version.h:805: CallbackType protect(callback_); Why are you protecting the callback? Is that because you expect the callback to have a scoped_refptr<ServiceWorkerVersion> to this? If so, that'll break if the callback instead has a bound scoped_ptr to some other class that holds a scoped_refptr<ServiceWorkerVersion>. In that case, once the callback is run, the scoped_ptr's ownership will be passed to the bound function, and subsequently destroyed unless the bound function does something with it. Hence ServiceWorkerVersion's refcount will be decremented, and possibly destroyed. Such callbacks can only be called once. Instead, it seems safer to protect whatever you want to protect directly: ... { scoped_refptr<Something> protect(something); // Essentially same code as what IPC_MESSAGE_FORWARD expands to. void* param = nullptr; if (!ResponseMessage::Dispatch(&message, &callback_, this, param, &CallbackType::Run)) message.set_dispatch_error(); } // At this point |this| can have been deleted, so don't do anything other // than returning. return true; }
https://codereview.chromium.org/1575283003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (left): https://codereview.chromium.org/1575283003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_version.cc:634: DCHECK_EQ(ACTIVATED, status()) << status(); On 2016/01/18 at 16:31:05, johnme wrote: > Both push and notifications have experienced a lot of subtle bugs about Service Workers not being in the right state. I'd rather keep this check in some form. Having a whitelist of events which don't need an activated Service Worker actually sounds quite reasonable, since this should be a small finite list (possibly just INSTALL and ACTIVATE? Why would we dispatch a MESSAGE to an inactive SW?). I think most of the subtle bugs have been adressed by the changes in ServiceWorkerContextWrapper, that pretty much now makes it impossible to get a worker which isn't in the right state. But given that the "handle functional event" algorithm does require an activating/activated service worker, I agree that the list of events without this requirements is likely going to be small. I created https://codereview.chromium.org/1604033003 to add this DCHECK. (We would dispatch a MESSAGE to an inactive SW because the spec says we should allow this (for example to allow communication between installing page and installing service worker during the oninstall event)). https://codereview.chromium.org/1575283003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/1575283003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_version.h:805: CallbackType protect(callback_); On 2016/01/18 at 17:57:05, johnme wrote: > Why are you protecting the callback? Is that because you expect the callback to have a scoped_refptr<ServiceWorkerVersion> to this? I am protecting the callback because that's what I need to keep alive till the end of its Run() method. Any parameters bound to the callback and passed by reference to the actual method being called are references to data owned by the Callback object. Without this protect(), FinishRequest called from within the callback would delete the EventResponseHandler, which held the last reference to the callback, and any further use of those parameters from within the callback would then be undefined behavior. I don't believe there is anything else that needs protecting here. By the time the callback returns |this| (the EventResponseHandler) can have been deleted, and so this method doesn't do anything else afterwards anyway. And if any code higher up in the stack needs something to still be alive after OnMessageReceived returns there is nothing this method can do to protect that anyway (and this isn't a change compared to the previous code either; we can protect objects to stay alive for the duration of the callback, not after we return).
Still lgtm https://codereview.chromium.org/1575283003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/1575283003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_version.h:805: CallbackType protect(callback_); On 2016/01/19 23:50:17, Marijn Kruisselbrink wrote: > On 2016/01/18 at 17:57:05, johnme wrote: > > Why are you protecting the callback? Is that because you expect the callback > to have a scoped_refptr<ServiceWorkerVersion> to this? > I am protecting the callback because that's what I need to keep alive till the > end of its Run() method. Any parameters bound to the callback and passed by > reference to the actual method being called are references to data owned by the > Callback object. Without this protect(), FinishRequest called from within the > callback would delete the EventResponseHandler, which held the last reference to > the callback, and any further use of those parameters from within the callback > would then be undefined behavior. > > I don't believe there is anything else that needs protecting here. By the time > the callback returns |this| (the EventResponseHandler) can have been deleted, > and so this method doesn't do anything else afterwards anyway. And if any code > higher up in the stack needs something to still be alive after OnMessageReceived > returns there is nothing this method can do to protect that anyway (and this > isn't a change compared to the previous code either; we can protect objects to > stay alive for the duration of the callback, not after we return). Ok, sounds reasonable. Who owns the |message|? Is is possible that could be deleted?
mek@chromium.org changed reviewers: + tsepez@chromium.org
johnme: I changed the response IPC to include the WebServiceWorkerEventResult, and then changed the rest of the CL to use the simplified method I added for the push event. Not a very significant change and the rest of the browser side notification click event code already handled the wait until rejected status. +tsepez for IPC OWNERS
Messages LGTM.
lgtm, thanks :)
The CQ bit was checked by mek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/1575283003/#ps120001 (title: "rebase and use DispatchSimpleEvent")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575283003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575283003/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Move notification click event dispatching out of ServiceWorkerVersion. BUG=570820 ========== to ========== Move notification click event dispatching out of ServiceWorkerVersion. BUG=570820 Committed: https://crrev.com/d9ef31156d4012582b3d551e584666f4524aa493 Cr-Commit-Position: refs/heads/master@{#371559} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d9ef31156d4012582b3d551e584666f4524aa493 Cr-Commit-Position: refs/heads/master@{#371559} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
