service worker: Don't crash if respondWith() is called in a microtask. #15851
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The RespondWithObserver and WaitUntilObserver interaction is tricky. Essentially WaitUntilObserver determines when the fetch event is finished. RespondWithObserver determines what the response to the event is. The response is expected to be determined before the event finishes. We had a bug where the fetch event could finish before the response was determined. RespondWithObserver depends on WaitUntilObserver to keep the event alive until the respondWith() promise settles, but in some cases WaitUntilObserver would silently ignore the promise. One easy repro was when respondWith() was called in a microtask immediately after event dispatch. This would cause a crash in DidDispatchFetchEvent() since the event ended but the response callback was still unconsumed. The fix is to ensure WaitUntilObserver observes the promise, and in cases that it won't, have RespondWithObserver properly handle the respondWith() by throwing an exception and not advancing to state kPending where it would wait for the promise to settle. So now, once DidDispatchEvent() is called, RespondWithObserver knows it has no outstanding promise and reports back that respondWith() was effectively not called, so the browser falls back to network. This also removes some early returns when the execution context is null, which made it more unclear when the promises would be observed. It should only be necessary to check the execution context when the context is about to be accessed. Bug: 941875 Change-Id: I14f008f7e76ec76d90fe25104cc6f3849808fb2f
wpt-pr-bot
approved these changes
Mar 15, 2019
|
Close this PR because the Chromium CL does not have exportable changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The RespondWithObserver and WaitUntilObserver interaction is tricky.
Essentially WaitUntilObserver determines when the fetch event is
finished. RespondWithObserver determines what the response to the
event is. The response is expected to be determined before the event
finishes.
We had a bug where the fetch event could finish before the response was
determined. RespondWithObserver depends on WaitUntilObserver to keep the
event alive until the respondWith() promise settles, but in some cases
WaitUntilObserver would silently ignore the promise.
One easy repro was when respondWith() was called in a microtask
immediately after event dispatch. This would cause a crash in
DidDispatchFetchEvent() since the event ended but the response callback
was still unconsumed.
The fix is to ensure WaitUntilObserver observes the promise, and in
cases that it won't, have RespondWithObserver properly handle the
respondWith() by throwing an exception and not advancing to state
kPending where it would wait for the promise to settle. So now, once
DidDispatchEvent() is called, RespondWithObserver knows it has no
outstanding promise and reports back that respondWith() was
effectively not called, so the browser falls back to network.
This also removes some early returns when the execution context is null,
which made it more unclear when the promises would be observed. It
should only be necessary to check the execution context when the context
is about to be accessed.
Bug: 941875
Change-Id: I14f008f7e76ec76d90fe25104cc6f3849808fb2f
Reviewed-on: https://chromium-review.googlesource.com/1523207
WPT-Export-Revision: 024e25a47b9eb11b5fb81744fccbe09d52cdda5e