Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396293002:
Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() (Closed)
|
Created:
4 years, 2 months ago by horo Modified:
4 years, 2 months ago Reviewers:
shimazu CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, nhiroki, falken, haraken, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, tzik Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed()
We don't need to (and should not) call this while the ServiceWorker context is being destroyed.
And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent.
(crbug.com/653403) This crash only happens when LifecycleNotifier::notifyContextDestroyed() calls
RespondWithObserver::contextDestroyed() before WaitUntilObserver::contextDestroyed().
BUG=653403, 649558, 629701
Committed: https://crrev.com/197a4f896f0434376b82895e7e72aacce16ec3ef
Cr-Commit-Position: refs/heads/master@{#424390}
Patch Set 1 #Patch Set 2 : add note #
Messages
Total messages: 30 (21 generated)
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Don't call WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() BUG=653403 ========== to ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() BUG=653403 ==========
Description was changed from ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() BUG=653403 ========== to ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. BUG=653403 ==========
Description was changed from ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. BUG=653403 ========== to ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. This crash only happens whenLifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver:::contextDestroyed(). BUG=653403 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. This crash only happens whenLifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver:::contextDestroyed(). BUG=653403 ========== to ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. This crash (crbug.com/653403) only happens whenLifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver:::contextDestroyed(). BUG=653403 ==========
Description was changed from ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. This crash (crbug.com/653403) only happens whenLifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver:::contextDestroyed(). BUG=653403 ========== to ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens whenLifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver:::contextDestroyed(). BUG=653403 ==========
Description was changed from ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens whenLifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver:::contextDestroyed(). BUG=653403 ========== to ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens when LifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver:::contextDestroyed(). BUG=653403 ==========
Description was changed from ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens whenLifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver:::contextDestroyed(). BUG=653403 ========== to ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens when LifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver:::contextDestroyed(). BUG=653403 ==========
Description was changed from ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens when LifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver:::contextDestroyed(). BUG=653403 ========== to ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens when LifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver::contextDestroyed(). BUG=653403 ==========
horo@chromium.org changed reviewers: + shimazu@chromium.org
shimazu@ Please review this.
On 2016/10/07 07:10:54, horo wrote: > shimazu@ > Please review this. How about adding some comments at decrementPendingActivity like "decrementPendingActivity should not be called when the ExtendableEvent has aborted."? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...
On 2016/10/07 07:26:41, shimazu wrote: > On 2016/10/07 07:10:54, horo wrote: > > shimazu@ > > Please review this. > > How about adding some comments at decrementPendingActivity like > "decrementPendingActivity should not be called when the ExtendableEvent has > aborted."? > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic... My understanding is: - If the ExtendableEvents (in general) have aborted, we should let WaitUntilObservers know about the abortion. - But in this case when the ExtendableEvent is the RespondWithObserver which handles FetchEvents and the ServiceWorker context is being destroyed, the WaitUntilObservers don't need to handle the abortion.
Description was changed from ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens when LifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver::contextDestroyed(). BUG=653403 ========== to ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens when LifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver::contextDestroyed(). BUG=653403,649558,629701 ==========
On 2016/10/07 08:04:21, horo wrote: > On 2016/10/07 07:26:41, shimazu wrote: > > On 2016/10/07 07:10:54, horo wrote: > > > shimazu@ > > > Please review this. > > > > How about adding some comments at decrementPendingActivity like > > "decrementPendingActivity should not be called when the ExtendableEvent has > > aborted."? > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic... > > My understanding is: > - If the ExtendableEvents (in general) have aborted, we should let > WaitUntilObservers know about the abortion. > - But in this case when the ExtendableEvent is the RespondWithObserver which > handles FetchEvents and the ServiceWorker context is being destroyed, the > WaitUntilObservers don't need to handle the abortion. Probably I made you confused because I used 'abort' wrongly. What I wanted to say (and I think these are mostly the same with your idea) was: - the lifetime of ExtendableEvents should be managed by WaitUntilObservers - RespondWithObserver can extend the lifetime through WaitUntilObservers - if an in-flight observer gets destroyed with the SW context, the ExtendableEvent should be discarded silently.
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/11 04:20:07, shimazu wrote: > On 2016/10/07 08:04:21, horo wrote: > > On 2016/10/07 07:26:41, shimazu wrote: > > > On 2016/10/07 07:10:54, horo wrote: > > > > shimazu@ > > > > Please review this. > > > > > > How about adding some comments at decrementPendingActivity like > > > "decrementPendingActivity should not be called when the ExtendableEvent has > > > aborted."? > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic... > > > > My understanding is: > > - If the ExtendableEvents (in general) have aborted, we should let > > WaitUntilObservers know about the abortion. > > - But in this case when the ExtendableEvent is the RespondWithObserver which > > handles FetchEvents and the ServiceWorker context is being destroyed, the > > WaitUntilObservers don't need to handle the abortion. > > Probably I made you confused because I used 'abort' wrongly. > What I wanted to say (and I think these are mostly the same with your idea) was: > - the lifetime of ExtendableEvents should be managed by WaitUntilObservers > - RespondWithObserver can extend the lifetime through WaitUntilObservers > - if an in-flight observer gets destroyed with the SW context, the > ExtendableEvent should be discarded silently. Added "note" in WaitUntilObserver.h. How about this?
lgtm, thanks! :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens when LifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver::contextDestroyed(). BUG=653403,649558,629701 ========== to ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens when LifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver::contextDestroyed(). BUG=653403,649558,629701 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens when LifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver::contextDestroyed(). BUG=653403,649558,629701 ========== to ========== Remove WaitUntilObserver::decrementPendingActivity() from RespondWithObserver::contextDestroyed() We don't need to (and should not) call this while the ServiceWorker context is being destroyed. And also this caused crashes in ServiceWorkerContextClient after mojofying FetchEvent. (crbug.com/653403) This crash only happens when LifecycleNotifier::notifyContextDestroyed() calls RespondWithObserver::contextDestroyed() before WaitUntilObserver::contextDestroyed(). BUG=653403,649558,629701 Committed: https://crrev.com/197a4f896f0434376b82895e7e72aacce16ec3ef Cr-Commit-Position: refs/heads/master@{#424390} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/197a4f896f0434376b82895e7e72aacce16ec3ef Cr-Commit-Position: refs/heads/master@{#424390} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
