Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2877543003:
[ServiceWorker] Allow waitUntil to be called multiple times asynchronously (Closed)
|
Created:
3 years, 7 months ago by leonhsl(Using Gerrit) Modified:
3 years, 7 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, blink-reviews-w3ctests_chromium.org, nhiroki, haraken, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, tzik Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ServiceWorker] Allow waitUntil to be called multiple times asynchronously
Spec:
https://w3c.github.io/ServiceWorker/#dom-extendableevent-waituntil
BUG=621440
TEST=blink_tests
external/wpt/service-workers/service-worker/extendable-event-async-waituntil.https.html
Review-Url: https://codereview.chromium.org/2877543003
Cr-Commit-Position: refs/heads/master@{#474630}
Committed: https://chromium.googlesource.com/chromium/src/+/eaf348dc9864d8073fe01472a91c33f21e3ec4b6
Patch Set 1 #Patch Set 2 #Patch Set 3 : Handle microtask cases #
Total comments: 2
Patch Set 4 : Enqueue new microtask #Patch Set 5 : Fix other tests using waitUntil incorrectly #
Total comments: 9
Patch Set 6 : Add EventDispatcherState::kDispatching #
Total comments: 2
Patch Set 7 : Address comments from shimazu@ #
Total comments: 22
Patch Set 8 : Part of comments from falken@ #
Total comments: 4
Patch Set 9 : Let RespondWithObserver call WaitUntilObserver::WaitUntil to add extend lifetime promise #
Total comments: 14
Patch Set 10 : Address comments from falken@ #Messages
Total messages: 93 (66 generated)
The CQ bit was checked by leon.han@intel.com 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...
The CQ bit was checked by leon.han@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
leon.han@intel.com changed reviewers: + falken@chromium.org, shimazu@chromium.org
This CL is incomplete now because 2 tests related with microtasks in extendable-event-async-waituntil.https.html are still failing. For that I posted a question to blink-dev at https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/r7VMDa5dlWc, I'm still a beginner on blink core so eager to your help.. Thanks!
On 2017/05/12 09:38:58, leonhsl wrote: > This CL is incomplete now because 2 tests related with microtasks in > extendable-event-async-waituntil.https.html are still failing. > For that I posted a question to blink-dev at > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/r7VMDa5dlWc, > I'm still a beginner on blink core so eager to your help.. Thanks! I looked briefly at the code but I'm also not so familiar with microtask processing in Blink. Let me know if you don't get a good explanation from blink-dev. One tip is it might help to explain the why of what you're doing, e.g., explain the test failures in async-waitUntil and what you're trying to implement.
The CQ bit was checked by leon.han@intel.com 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...
Hi, falken@, I tried my best for now and uploaded ps#3, which modified WaitUntilObserver impl and added 2 new tests related with respondWith promise microtasks, and now the only one failed test is current-extension-expired-same-microtask-turn: ' FAIL Test calling waitUntil with an existing extension promise handler succeeds assert_unreached: unexpected rejection: assert_equals: expected "OK" but got "InvalidStateError" Reached unreachable code ' Case A: current-extension-expired-same-microtask-turn asserts that waitUntil is allowed inside a microtask of a promise. Case B: current-extension-expired-same-microtask-turn-extra asserts that waitUntil is NOT allowed inside a microtask generated by the microtask of Case A on above. Usually microtasks are consolidated to run, meaning that if one microtask will queue another new microtask, all queued microtasks are executed sequentially until no any microtask alive, so I think all these microtasks in a run should not be considered having big differences. (Actually with current V8 APIs we can't know current running microtask is a parent one or a child/grandchild one) So I'm confused why Case A and Case B assert so differently. Would you help to clarify more context/knowledge/justification about these 2 test cases? Thanks! https://codereview.chromium.org/2877543003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/extendable-event-async-waituntil.js (right): https://codereview.chromium.org/2877543003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/extendable-event-async-waituntil.js:62: sync_waituntil(event).then(reportResultExpecting('OK')); waitUntil can succeed for this test, its counterpart test 'current-extension-expired-same-microtask-turn' also asserts waitUntil should be allowed. https://codereview.chromium.org/2877543003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/extendable-event-async-waituntil.js:67: async_microtask_waituntil(event).then(reportResultExpecting('OK')); waitUntil can succeed for this test, but its counterpart test 'current-extension-expired-same-microtask-turn-extra' asserts waitUntil should NOT be allowed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/05/18 08:40:31, leonhsl wrote:
> Hi, falken@, I tried my best for now and uploaded ps#3, which modified
> WaitUntilObserver impl and added 2 new tests related with respondWith promise
> microtasks, and now the only one failed test is
> current-extension-expired-same-microtask-turn:
> '
> FAIL Test calling waitUntil with an existing extension promise handler
succeeds
> assert_unreached: unexpected rejection: assert_equals: expected "OK" but got
> "InvalidStateError" Reached unreachable code
> '
>
> Case A: current-extension-expired-same-microtask-turn
> asserts that waitUntil is allowed inside a microtask of a promise.
> Case B: current-extension-expired-same-microtask-turn-extra
> asserts that waitUntil is NOT allowed inside a microtask generated by the
> microtask of Case A on above.
>
> Usually microtasks are consolidated to run, meaning that if one microtask will
> queue another new microtask, all queued microtasks are executed sequentially
> until no any microtask alive, so I think all these microtasks in a run should
> not be considered having big differences. (Actually with current V8 APIs we
> can't know current running microtask is a parent one or a child/grandchild
one)
>
> So I'm confused why Case A and Case B assert so differently. Would you help to
> clarify more context/knowledge/justification about these 2 test cases? Thanks!
It's just the API design. The API design is such that a developer is allowed to
do A but not B.
// A
e.waitUntil(p);
p.then(() => {
e.waitUntil(r); // OK: wait for r too
});
// B
e.waitUntil(p);
p.then(() => {
Promise.resolve().then(() => {
e.waitUntil(r); // NOT OK: should throw
});
i.e., you can hang another waitUntil on immediately in the .then() of a previous
waitUntil promise, but you can't do it after that .then() finishes. See
https://github.com/w3c/ServiceWorker/issues/1039#issuecomment-271892473
I'm not sure the correct way to implement this in Blink.
On 2017/05/19 07:41:24, falken wrote:
> On 2017/05/18 08:40:31, leonhsl wrote:
> > Hi, falken@, I tried my best for now and uploaded ps#3, which modified
> > WaitUntilObserver impl and added 2 new tests related with respondWith
promise
> > microtasks, and now the only one failed test is
> > current-extension-expired-same-microtask-turn:
> > '
> > FAIL Test calling waitUntil with an existing extension promise handler
> succeeds
> > assert_unreached: unexpected rejection: assert_equals: expected "OK" but got
> > "InvalidStateError" Reached unreachable code
> > '
> >
> > Case A: current-extension-expired-same-microtask-turn
> > asserts that waitUntil is allowed inside a microtask of a promise.
> > Case B: current-extension-expired-same-microtask-turn-extra
> > asserts that waitUntil is NOT allowed inside a microtask generated by the
> > microtask of Case A on above.
> >
> > Usually microtasks are consolidated to run, meaning that if one microtask
will
> > queue another new microtask, all queued microtasks are executed sequentially
> > until no any microtask alive, so I think all these microtasks in a run
should
> > not be considered having big differences. (Actually with current V8 APIs we
> > can't know current running microtask is a parent one or a child/grandchild
> one)
> >
> > So I'm confused why Case A and Case B assert so differently. Would you help
to
> > clarify more context/knowledge/justification about these 2 test cases?
Thanks!
>
> It's just the API design. The API design is such that a developer is allowed
to
> do A but not B.
>
> // A
> e.waitUntil(p);
>
> p.then(() => {
> e.waitUntil(r); // OK: wait for r too
> });
>
> // B
> e.waitUntil(p);
>
> p.then(() => {
> Promise.resolve().then(() => {
> e.waitUntil(r); // NOT OK: should throw
> });
>
> i.e., you can hang another waitUntil on immediately in the .then() of a
previous
> waitUntil promise, but you can't do it after that .then() finishes. See
> https://github.com/w3c/ServiceWorker/issues/1039#issuecomment-271892473
>
> I'm not sure the correct way to implement this in Blink.
Would it work if in WaitUntilObserver::ThenFunction::Call we instead queue a
task (microtask) that calls OnPromiseFulfilled/Rejected(), instead of doing that
synchronously in ThenFunction::Call?
On 2017/05/19 07:45:42, falken wrote:
> On 2017/05/19 07:41:24, falken wrote:
> > On 2017/05/18 08:40:31, leonhsl wrote:
> > > Hi, falken@, I tried my best for now and uploaded ps#3, which modified
> > > WaitUntilObserver impl and added 2 new tests related with respondWith
> promise
> > > microtasks, and now the only one failed test is
> > > current-extension-expired-same-microtask-turn:
> > > '
> > > FAIL Test calling waitUntil with an existing extension promise handler
> > succeeds
> > > assert_unreached: unexpected rejection: assert_equals: expected "OK" but
got
> > > "InvalidStateError" Reached unreachable code
> > > '
> > >
> > > Case A: current-extension-expired-same-microtask-turn
> > > asserts that waitUntil is allowed inside a microtask of a promise.
> > > Case B: current-extension-expired-same-microtask-turn-extra
> > > asserts that waitUntil is NOT allowed inside a microtask generated by the
> > > microtask of Case A on above.
> > >
> > > Usually microtasks are consolidated to run, meaning that if one microtask
> will
> > > queue another new microtask, all queued microtasks are executed
sequentially
> > > until no any microtask alive, so I think all these microtasks in a run
> should
> > > not be considered having big differences. (Actually with current V8 APIs
we
> > > can't know current running microtask is a parent one or a child/grandchild
> > one)
> > >
> > > So I'm confused why Case A and Case B assert so differently. Would you
help
> to
> > > clarify more context/knowledge/justification about these 2 test cases?
> Thanks!
> >
> > It's just the API design. The API design is such that a developer is allowed
> to
> > do A but not B.
> >
> > // A
> > e.waitUntil(p);
> >
> > p.then(() => {
> > e.waitUntil(r); // OK: wait for r too
> > });
> >
> > // B
> > e.waitUntil(p);
> >
> > p.then(() => {
> > Promise.resolve().then(() => {
> > e.waitUntil(r); // NOT OK: should throw
> > });
> >
> > i.e., you can hang another waitUntil on immediately in the .then() of a
> previous
> > waitUntil promise, but you can't do it after that .then() finishes. See
> > https://github.com/w3c/ServiceWorker/issues/1039#issuecomment-271892473
> >
> > I'm not sure the correct way to implement this in Blink.
>
> Would it work if in WaitUntilObserver::ThenFunction::Call we instead queue a
> task (microtask) that calls OnPromiseFulfilled/Rejected(), instead of doing
that
> synchronously in ThenFunction::Call?
I like this idea! Thank you!
This is utilizing the mechanism that new microtask is always put at the end of
microtask queque. Assuming this could work, let me take a try.
The CQ bit was checked by leon.han@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by leon.han@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by leon.han@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com 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...
The CQ bit was checked by leon.han@intel.com 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...
Patchset #5 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:100001) has been deleted
I think ps#5 is ready for formal review now, thanks a lot for help and PTAnL.
https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.h (right): https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.h:66: bool created_by_script_; const? https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:132: v8::MicrotasksScope::IsRunningMicrotasks(script_state->GetIsolate())) { Could you explain the purpose of this branch? I might be missing something, but I feel this is a bit weird. I'm now investigating around this.
Thanks a lot for review! https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.h (right): https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.h:66: bool created_by_script_; On 2017/05/22 03:48:41, shimazu wrote: > const? Acknowledged. https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:132: v8::MicrotasksScope::IsRunningMicrotasks(script_state->GetIsolate())) { On 2017/05/22 03:48:41, shimazu wrote: > Could you explain the purpose of this branch? > I might be missing something, but I feel this is a bit weird. > I'm now investigating around this. This is to forbid calling waitUntil in such a case: onXXX = function(e) { Promise.resolve().then(() => { e.waitUntil(XXX); }); }; WaitUntilObserver::DidDispatchEvent() is called after EventTarget::DispatchEvent() returned, which means that both event handler execution itself and those microtasks generated by it have completed. But we should forbid calling waitUntil during event handler's microtasks if there has no any other extend lifetime promise. wpt test case: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...
https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:132: v8::MicrotasksScope::IsRunningMicrotasks(script_state->GetIsolate())) { On 2017/05/22 05:01:43, leonhsl wrote: > On 2017/05/22 03:48:41, shimazu wrote: > > Could you explain the purpose of this branch? > > I might be missing something, but I feel this is a bit weird. > > I'm now investigating around this. > > This is to forbid calling waitUntil in such a case: > onXXX = function(e) { > Promise.resolve().then(() => { e.waitUntil(XXX); }); > }; > > WaitUntilObserver::DidDispatchEvent() is called after > EventTarget::DispatchEvent() returned, which means that both event handler > execution itself and those microtasks generated by it have completed. > But we should forbid calling waitUntil during event handler's microtasks if > there has no any other extend lifetime promise. > > wpt test case: > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... I see, I could understand what this is for, thanks! IIUC, this is necessary for implemeting "Note" of the step 1 in 4.4.1 on the spec. How about adding pending_promises_count() to WaitUntilObserver and using switch to clarify why microtasks should throw? I mean, adding pending_promises_count() like this: ``` int WaitUntilObserver::pending_promises_count() { return (state_ == State::kInitial) ? pending_activity_ - 1 : pending_activity_; } ``` and using switch like the following: ``` if (pending_promises_count() == 0) { switch(event_dispatch_state_) { case EventDispatchState::kInitial: // WaitUntil is valid though pending_promises_count == 0 when it's in the // event handler. if (!v8::MicrotasksScope::IsRunningMicrotasks( script_state->GetIsolate())) break; case EventDispatchState::kCompleted: case EventDispatchState::kFailed: exception_state.ThrowDOMException( kInvalidStateError, "SOME GOOD MESSAGE HERE"); return; } } ```
Thanks a lot for review! https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:83: WrapPersistent(observer_.Get()))); I'm not so familiar with WTF::Bind compared with base::Bind, WrapPersistent() seems be like base::Unretained()? Not sure whether it can ensure |observer_| to be valid long enough here... https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:132: v8::MicrotasksScope::IsRunningMicrotasks(script_state->GetIsolate())) { On 2017/05/22 08:09:05, shimazu wrote: > On 2017/05/22 05:01:43, leonhsl wrote: > > On 2017/05/22 03:48:41, shimazu wrote: > > > Could you explain the purpose of this branch? > > > I might be missing something, but I feel this is a bit weird. > > > I'm now investigating around this. > > > > This is to forbid calling waitUntil in such a case: > > onXXX = function(e) { > > Promise.resolve().then(() => { e.waitUntil(XXX); }); > > }; > > > > WaitUntilObserver::DidDispatchEvent() is called after > > EventTarget::DispatchEvent() returned, which means that both event handler > > execution itself and those microtasks generated by it have completed. > > But we should forbid calling waitUntil during event handler's microtasks if > > there has no any other extend lifetime promise. > > > > wpt test case: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... > > I see, I could understand what this is for, thanks! > IIUC, this is necessary for implemeting "Note" of the step 1 in 4.4.1 on the > spec. > How about adding pending_promises_count() to WaitUntilObserver and using switch > to clarify why microtasks should throw? > > I mean, adding pending_promises_count() like this: > ``` > int WaitUntilObserver::pending_promises_count() { > return (state_ == State::kInitial) ? > pending_activity_ - 1 : pending_activity_; > } > ``` > > and using switch like the following: > ``` > if (pending_promises_count() == 0) { > switch(event_dispatch_state_) { > case EventDispatchState::kInitial: > // WaitUntil is valid though pending_promises_count == 0 when it's in > the > // event handler. > if (!v8::MicrotasksScope::IsRunningMicrotasks( > script_state->GetIsolate())) > break; > case EventDispatchState::kCompleted: > case EventDispatchState::kFailed: > exception_state.ThrowDOMException( > kInvalidStateError, "SOME GOOD MESSAGE HERE"); > return; > } > } > ``` The above suggestion reminded me, I considered a lot about scenario around this class, and got another option :) - Add a new EventDispatchState::kDispatching enum value, let |event_dispatch_state_| be this value to indicate status between WaitUntilObserver::WillDispatchEvent() and WaitUntilObserver::DidDispatchEvent(). - Rename {Increment,Decrement}PendingActivity to {Increment,Decrement}PendingPromise, rename pending_activity_ to pending_promise_, use them only for extend lifetime promises(for waitUntil, respondWith etc.), not for event handler. - Thus we can make code clearer to understand status of event handler VS. extend lifetime promises. WDYT? Thanks!
On 2017/05/22 13:23:02, leonhsl wrote: > Thanks a lot for review! > > https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp > (right): > > https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:83: > WrapPersistent(observer_.Get()))); > I'm not so familiar with WTF::Bind compared with base::Bind, WrapPersistent() > seems be like base::Unretained()? Not sure whether it can ensure |observer_| to > be valid long enough here... > > https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:132: > v8::MicrotasksScope::IsRunningMicrotasks(script_state->GetIsolate())) { > On 2017/05/22 08:09:05, shimazu wrote: > > On 2017/05/22 05:01:43, leonhsl wrote: > > > On 2017/05/22 03:48:41, shimazu wrote: > > > > Could you explain the purpose of this branch? > > > > I might be missing something, but I feel this is a bit weird. > > > > I'm now investigating around this. > > > > > > This is to forbid calling waitUntil in such a case: > > > onXXX = function(e) { > > > Promise.resolve().then(() => { e.waitUntil(XXX); }); > > > }; > > > > > > WaitUntilObserver::DidDispatchEvent() is called after > > > EventTarget::DispatchEvent() returned, which means that both event handler > > > execution itself and those microtasks generated by it have completed. > > > But we should forbid calling waitUntil during event handler's microtasks if > > > there has no any other extend lifetime promise. > > > > > > wpt test case: > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... > > > > I see, I could understand what this is for, thanks! > > IIUC, this is necessary for implemeting "Note" of the step 1 in 4.4.1 on the > > spec. > > How about adding pending_promises_count() to WaitUntilObserver and using > switch > > to clarify why microtasks should throw? > > > > I mean, adding pending_promises_count() like this: > > ``` > > int WaitUntilObserver::pending_promises_count() { > > return (state_ == State::kInitial) ? > > pending_activity_ - 1 : pending_activity_; > > } > > ``` > > > > and using switch like the following: > > ``` > > if (pending_promises_count() == 0) { > > switch(event_dispatch_state_) { > > case EventDispatchState::kInitial: > > // WaitUntil is valid though pending_promises_count == 0 when it's in > > the > > // event handler. > > if (!v8::MicrotasksScope::IsRunningMicrotasks( > > script_state->GetIsolate())) > > break; > > case EventDispatchState::kCompleted: > > case EventDispatchState::kFailed: > > exception_state.ThrowDOMException( > > kInvalidStateError, "SOME GOOD MESSAGE HERE"); > > return; > > } > > } > > ``` > > The above suggestion reminded me, I considered a lot about scenario around this > class, and got another option :) > - Add a new EventDispatchState::kDispatching enum value, let > |event_dispatch_state_| be this value to indicate status between > WaitUntilObserver::WillDispatchEvent() and > WaitUntilObserver::DidDispatchEvent(). > - Rename {Increment,Decrement}PendingActivity to > {Increment,Decrement}PendingPromise, rename pending_activity_ to > pending_promise_, use them only for extend lifetime promises(for waitUntil, > respondWith etc.), not for event handler. > - Thus we can make code clearer to understand status of event handler VS. > extend lifetime promises. > > WDYT? Thanks! This sounds better because it's aligning more closely to the spec. The spec is pretty simple: 1. If the pending promises count is zero and the dispatch flag is unset, then: 1. Throw an "InvalidStateError" exception. 2. Abort these steps. 2. Add f to the extend lifetime promises. 3. Increment the pending promises count by one. 4. Upon fulfillment or rejection of f, queue a microtask to run these substeps: 1. Decrement the pending promises count by one. Ideally we would just implement this. But I guess Blink's dispatch event functions may make it hard to map "dispatch flag" to the implementation? I suspect the spec assumes "dispatch flag" is unset when you exit the event handler and before microtasks run.
On 2017/05/23 00:52:08, falken wrote: > On 2017/05/22 13:23:02, leonhsl wrote: > > Thanks a lot for review! > > > > > https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp > > (right): > > > > > https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:83: > > WrapPersistent(observer_.Get()))); > > I'm not so familiar with WTF::Bind compared with base::Bind, WrapPersistent() > > seems be like base::Unretained()? Not sure whether it can ensure |observer_| > to > > be valid long enough here... > > > > > https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:132: > > v8::MicrotasksScope::IsRunningMicrotasks(script_state->GetIsolate())) { > > On 2017/05/22 08:09:05, shimazu wrote: > > > On 2017/05/22 05:01:43, leonhsl wrote: > > > > On 2017/05/22 03:48:41, shimazu wrote: > > > > > Could you explain the purpose of this branch? > > > > > I might be missing something, but I feel this is a bit weird. > > > > > I'm now investigating around this. > > > > > > > > This is to forbid calling waitUntil in such a case: > > > > onXXX = function(e) { > > > > Promise.resolve().then(() => { e.waitUntil(XXX); }); > > > > }; > > > > > > > > WaitUntilObserver::DidDispatchEvent() is called after > > > > EventTarget::DispatchEvent() returned, which means that both event handler > > > > execution itself and those microtasks generated by it have completed. > > > > But we should forbid calling waitUntil during event handler's microtasks > if > > > > there has no any other extend lifetime promise. > > > > > > > > wpt test case: > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... > > > > > > I see, I could understand what this is for, thanks! > > > IIUC, this is necessary for implemeting "Note" of the step 1 in 4.4.1 on the > > > spec. > > > How about adding pending_promises_count() to WaitUntilObserver and using > > switch > > > to clarify why microtasks should throw? > > > > > > I mean, adding pending_promises_count() like this: > > > ``` > > > int WaitUntilObserver::pending_promises_count() { > > > return (state_ == State::kInitial) ? > > > pending_activity_ - 1 : pending_activity_; > > > } > > > ``` > > > > > > and using switch like the following: > > > ``` > > > if (pending_promises_count() == 0) { > > > switch(event_dispatch_state_) { > > > case EventDispatchState::kInitial: > > > // WaitUntil is valid though pending_promises_count == 0 when it's > in > > > the > > > // event handler. > > > if (!v8::MicrotasksScope::IsRunningMicrotasks( > > > script_state->GetIsolate())) > > > break; > > > case EventDispatchState::kCompleted: > > > case EventDispatchState::kFailed: > > > exception_state.ThrowDOMException( > > > kInvalidStateError, "SOME GOOD MESSAGE HERE"); > > > return; > > > } > > > } > > > ``` > > > > The above suggestion reminded me, I considered a lot about scenario around > this > > class, and got another option :) > > - Add a new EventDispatchState::kDispatching enum value, let > > |event_dispatch_state_| be this value to indicate status between > > WaitUntilObserver::WillDispatchEvent() and > > WaitUntilObserver::DidDispatchEvent(). > > - Rename {Increment,Decrement}PendingActivity to > > {Increment,Decrement}PendingPromise, rename pending_activity_ to > > pending_promise_, use them only for extend lifetime promises(for waitUntil, > > respondWith etc.), not for event handler. > > - Thus we can make code clearer to understand status of event handler VS. > > extend lifetime promises. > > > > WDYT? Thanks! > > This sounds better because it's aligning more closely to the spec. The spec is > pretty simple: > > 1. If the pending promises count is zero and the dispatch flag is unset, then: > 1. Throw an "InvalidStateError" exception. > 2. Abort these steps. > 2. Add f to the extend lifetime promises. > 3. Increment the pending promises count by one. > 4. Upon fulfillment or rejection of f, queue a microtask to run these substeps: > 1. Decrement the pending promises count by one. > > Ideally we would just implement this. But I guess Blink's dispatch event > functions may make it hard to map "dispatch flag" to the implementation? I > suspect the spec assumes "dispatch flag" is unset when you exit the event > handler and before microtasks run. Yeah, exactly! Blink's dispatch event functions can't tell us the time point("dispatch flag" is unset) when exit the event handler and before microtasks run, so instead we have to use v8::MicrotasksScope::IsRunningMicrotasks(), I learnt about this from blink-dev@ discussions ;-)
On 2017/05/23 01:05:06, leonhsl wrote: > On 2017/05/23 00:52:08, falken wrote: > > On 2017/05/22 13:23:02, leonhsl wrote: > > > Thanks a lot for review! > > > > > > > > > https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:83: > > > WrapPersistent(observer_.Get()))); > > > I'm not so familiar with WTF::Bind compared with base::Bind, > WrapPersistent() > > > seems be like base::Unretained()? Not sure whether it can ensure |observer_| > > to > > > be valid long enough here... > > > > > > > > > https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:132: > > > v8::MicrotasksScope::IsRunningMicrotasks(script_state->GetIsolate())) { > > > On 2017/05/22 08:09:05, shimazu wrote: > > > > On 2017/05/22 05:01:43, leonhsl wrote: > > > > > On 2017/05/22 03:48:41, shimazu wrote: > > > > > > Could you explain the purpose of this branch? > > > > > > I might be missing something, but I feel this is a bit weird. > > > > > > I'm now investigating around this. > > > > > > > > > > This is to forbid calling waitUntil in such a case: > > > > > onXXX = function(e) { > > > > > Promise.resolve().then(() => { e.waitUntil(XXX); }); > > > > > }; > > > > > > > > > > WaitUntilObserver::DidDispatchEvent() is called after > > > > > EventTarget::DispatchEvent() returned, which means that both event > handler > > > > > execution itself and those microtasks generated by it have completed. > > > > > But we should forbid calling waitUntil during event handler's microtasks > > if > > > > > there has no any other extend lifetime promise. > > > > > > > > > > wpt test case: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... > > > > > > > > I see, I could understand what this is for, thanks! > > > > IIUC, this is necessary for implemeting "Note" of the step 1 in 4.4.1 on > the > > > > spec. > > > > How about adding pending_promises_count() to WaitUntilObserver and using > > > switch > > > > to clarify why microtasks should throw? > > > > > > > > I mean, adding pending_promises_count() like this: > > > > ``` > > > > int WaitUntilObserver::pending_promises_count() { > > > > return (state_ == State::kInitial) ? > > > > pending_activity_ - 1 : pending_activity_; > > > > } > > > > ``` > > > > > > > > and using switch like the following: > > > > ``` > > > > if (pending_promises_count() == 0) { > > > > switch(event_dispatch_state_) { > > > > case EventDispatchState::kInitial: > > > > // WaitUntil is valid though pending_promises_count == 0 when it's > > in > > > > the > > > > // event handler. > > > > if (!v8::MicrotasksScope::IsRunningMicrotasks( > > > > script_state->GetIsolate())) > > > > break; > > > > case EventDispatchState::kCompleted: > > > > case EventDispatchState::kFailed: > > > > exception_state.ThrowDOMException( > > > > kInvalidStateError, "SOME GOOD MESSAGE HERE"); > > > > return; > > > > } > > > > } > > > > ``` > > > > > > The above suggestion reminded me, I considered a lot about scenario around > > this > > > class, and got another option :) > > > - Add a new EventDispatchState::kDispatching enum value, let > > > |event_dispatch_state_| be this value to indicate status between > > > WaitUntilObserver::WillDispatchEvent() and > > > WaitUntilObserver::DidDispatchEvent(). > > > - Rename {Increment,Decrement}PendingActivity to > > > {Increment,Decrement}PendingPromise, rename pending_activity_ to > > > pending_promise_, use them only for extend lifetime promises(for waitUntil, > > > respondWith etc.), not for event handler. > > > - Thus we can make code clearer to understand status of event handler VS. > > > extend lifetime promises. > > > > > > WDYT? Thanks! > > > > This sounds better because it's aligning more closely to the spec. The spec is > > pretty simple: > > > > 1. If the pending promises count is zero and the dispatch flag is unset, then: > > 1. Throw an "InvalidStateError" exception. > > 2. Abort these steps. > > 2. Add f to the extend lifetime promises. > > 3. Increment the pending promises count by one. > > 4. Upon fulfillment or rejection of f, queue a microtask to run these > substeps: > > 1. Decrement the pending promises count by one. > > > > Ideally we would just implement this. But I guess Blink's dispatch event > > functions may make it hard to map "dispatch flag" to the implementation? I > > suspect the spec assumes "dispatch flag" is unset when you exit the event > > handler and before microtasks run. > > Yeah, exactly! > Blink's dispatch event functions can't tell us the time point("dispatch flag" is > unset) when exit the event handler and before microtasks run, so instead we have > to use v8::MicrotasksScope::IsRunningMicrotasks(), I learnt about this from > blink-dev@ discussions ;-) If you can implement the idea of adding EventDispatcherState::kDispatching, it sounds better, but I didn't understand what's the difference between kInitial and kDispatching (actually I feel kDispatching/kDispatched is better naming than kInitial/kCompleted, though). It's also good to add "MaybeCompleteEvent" to notify the end of the lifetime of the event and calling it in DidDispatchEvent and DecrementPendingPromise when you implement "{Increment,Decrement}PendingPromise, rename pending_activity_ to pending_promise_, use them only for extend lifetime promises(for waitUntil, respondWith etc.), not for event handler.".
The CQ bit was checked by leon.han@intel.com 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...
Thank you for all the help! Uploaded ps#6, PTAnL, Thanks. I think kInitial indicates the initial state before WillDispatchEvent(), while kDispatching indicates the state after WillDispatchEvent() and before DidDispatchEvent().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with some comment. Please wait for falken's comment also. https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:83: WrapPersistent(observer_.Get()))); On 2017/05/22 13:23:02, leonhsl wrote: > I'm not so familiar with WTF::Bind compared with base::Bind, WrapPersistent() > seems be like base::Unretained()? Not sure whether it can ensure |observer_| to > be valid long enough here... I think it's good as is because the task should have the ownership of the observer instead of this ThenFunction. https://codereview.chromium.org/2877543003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:137: "Although the event handler is not finished yet, it's running " I think it's okay to have the same message with the case of kDispatched/kFailed because the event handler itself has finished here even though the subsequent microtasks are running.
The CQ bit was checked by leon.han@intel.com 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...
https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:83: WrapPersistent(observer_.Get()))); On 2017/05/23 09:08:43, shimazu wrote: > On 2017/05/22 13:23:02, leonhsl wrote: > > I'm not so familiar with WTF::Bind compared with base::Bind, WrapPersistent() > > seems be like base::Unretained()? Not sure whether it can ensure |observer_| > to > > be valid long enough here... > > I think it's good as is because the task should have the ownership of the > observer instead of this ThenFunction. Got it. Thanks! https://codereview.chromium.org/2877543003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:137: "Although the event handler is not finished yet, it's running " On 2017/05/23 09:08:43, shimazu wrote: > I think it's okay to have the same message with the case of kDispatched/kFailed > because the event handler itself has finished here even though the subsequent > microtasks are running. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
falken@chromium.org changed reviewers: + kinuko@chromium.org
Thanks for the patch. This is looking pretty good. Also +kinuko in case you have time to look at this as a core Blink owner https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/extendable-event-async-waituntil.js (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/extendable-event-async-waituntil.js:64: })); did these tests catch an implementation bug? i wonder if it'd be more useful to test: response = Promise.resolve(new Response('response')); event.respondWith(response); response.then(sync_waituntil); // ok response.then(async_waituntil); // fails this way it tests that you can do waitUntil() in the microtask for the promise passed to respondWith. Maybe we want both? It'd be good to have negative tests, i.e., that InvalidStateError is thrown. i found the new Response('OK') confusing since the 'OK' in the response is not actually used, AFAICT. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/extendable-event-async-waituntil.js:69: })); nit: indentation looks wrong, 62-63 and 67-68 should be indented an additional 2 spaces https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/sw-test-helpers.js (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/sw-test-helpers.js:66: e.waitUntil(Promise.resolve()); This is pretty tricky and I'm not sure with this change the tests in resources/notificationclick-can-focus.js are still testing what they intended to test. But I don't have any ideas. I think the people originally involved with this test are no longer working on service worker. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:11: e.waitUntil(Promise.resolve()); ditto: this is tricky and i'm not sure what the implications are for the tests that use this. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.cpp (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.cpp:59: return; Is this part of the spec? Can you link to it and have a WPT test? https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.cpp:74: created_by_script_(false), I feel there is a more idiomatic way to do this in Blink. event.isTrusted? Or can you just check if observer is nullptr? https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:70: // https://w3c.github.io/ServiceWorker/#dom-extendableevent-waituntil Add quote: "Upon fulfillment or rejection of f, queue a microtask to run these substeps: Decrement the pending promises count by one." https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:138: // Although the event handler is not finished yet, it's running microtask, I'd like this explanation to better explain what you said in the comment thread here about how didDispatchEvent() is called after both the event handler execution finished and microtasks queued by the event handler execution finished. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:212: case EventDispatchState::kDispatched: // Still waiting for a promise, do not complete the event. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:214: return; // Dispatch finished and there are no pending promises, complete the event. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:97: int pending_promise_ = 0; pending_promises_
The CQ bit was checked by leon.han@intel.com 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...
Thanks a lot for review! https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/extendable-event-async-waituntil.js (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/extendable-event-async-waituntil.js:64: })); On 2017/05/24 08:24:18, falken wrote: > did these tests catch an implementation bug? > > i wonder if it'd be more useful to test: > > response = Promise.resolve(new Response('response')); > event.respondWith(response); > response.then(sync_waituntil); // ok > response.then(async_waituntil); // fails > > this way it tests that you can do waitUntil() in the microtask for the promise > passed to respondWith. Maybe we want both? > > It'd be good to have negative tests, i.e., that InvalidStateError is thrown. > > i found the new Response('OK') confusing since the 'OK' in the response is not > actually used, AFAICT. I thought test coverage for respondWith is not enough so I created such 2 new tests. Yes I do think your suggested tests are better, and sync_waituntil should be OK and async_waituntil should throw, given the description in spec https://w3c.github.io/ServiceWorker/#dom-fetchevent-respondwith: ' Note: event.respondWith(r) extends the lifetime of the event by default as if event.waitUntil(r) is called. ' I'll revise the tests and try to solve potential bug around them. Thanks! https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/extendable-event-async-waituntil.js:69: })); On 2017/05/24 08:24:18, falken wrote: > nit: indentation looks wrong, 62-63 and 67-68 should be indented an additional 2 > spaces Acknowledged. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/sw-test-helpers.js (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/sw-test-helpers.js:66: e.waitUntil(Promise.resolve()); On 2017/05/24 08:24:18, falken wrote: > This is pretty tricky and I'm not sure with this change the tests in > resources/notificationclick-can-focus.js are still testing what they intended to > test. > > But I don't have any ideas. I think the people originally involved with this > test are no longer working on service worker. I believe this change does not change any behavior of those tests using this synthesizeNotificationClick function. They are calling waitUntil inside the returned promise's resolver function, which executes on a microtask after completion of the event handler self's execution. This is allowed before this CL, but now it's forbidden if has no any other outstanding extend lifetime promise at that time, so I added such a new call to waitUntil to ensure that. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.cpp (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.cpp:59: return; On 2017/05/24 08:24:18, falken wrote: > Is this part of the spec? Can you link to it and have a WPT test? Done. Linked to an existing WPT test. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.cpp:74: created_by_script_(false), On 2017/05/24 08:24:18, falken wrote: > I feel there is a more idiomatic way to do this in Blink. event.isTrusted? Or > can you just check if observer is nullptr? Done. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:70: // https://w3c.github.io/ServiceWorker/#dom-extendableevent-waituntil On 2017/05/24 08:24:18, falken wrote: > Add quote: > > "Upon fulfillment or rejection of f, queue a microtask to run these substeps: > > Decrement the pending promises count by one." Done. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:138: // Although the event handler is not finished yet, it's running microtask, On 2017/05/24 08:24:18, falken wrote: > I'd like this explanation to better explain what you said in the comment thread > here about how didDispatchEvent() is called after both the event handler > execution finished and microtasks queued by the event handler execution > finished. Done. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:212: case EventDispatchState::kDispatched: On 2017/05/24 08:24:18, falken wrote: > // Still waiting for a promise, do not complete the event. Done. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:214: return; On 2017/05/24 08:24:18, falken wrote: > // Dispatch finished and there are no pending promises, complete the event. Done. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:97: int pending_promise_ = 0; On 2017/05/24 08:24:18, falken wrote: > pending_promises_ Done.
lgtm tricky, but looks like this is doing what's expected. https://codereview.chromium.org/2877543003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:142: // finished, it's hard to get the presise time point between the 2 nit: presise -> precise https://codereview.chromium.org/2877543003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:146: // finished, in such case if there has no any extend lifetime promises are has -> have? 'if there aren't any outstanding lifetime-extending promises' ? (Is 'extend lifetime promises' a spec term?)
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 leon.han@intel.com 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...
The CQ bit was checked by leon.han@intel.com 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...
Patchset #9 (id:200001) has been deleted
Thank you all very much for your kindly review! At the same time I have to say sorry that I need your further review on ps#9, which changed WPT tests for respondWith and modified RespondWithObserver impl to make tests PASS. I missed this because the tests for respondWith added by me were not so good.. https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/extendable-event-async-waituntil.js (right): https://codereview.chromium.org/2877543003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/extendable-event-async-waituntil.js:64: })); On 2017/05/25 00:16:39, leonhsl wrote: > On 2017/05/24 08:24:18, falken wrote: > > did these tests catch an implementation bug? > > > > i wonder if it'd be more useful to test: > > > > response = Promise.resolve(new Response('response')); > > event.respondWith(response); > > response.then(sync_waituntil); // ok > > response.then(async_waituntil); // fails > > > > this way it tests that you can do waitUntil() in the microtask for the promise > > passed to respondWith. Maybe we want both? > > > > It'd be good to have negative tests, i.e., that InvalidStateError is thrown. > > > > i found the new Response('OK') confusing since the 'OK' in the response is not > > actually used, AFAICT. > > I thought test coverage for respondWith is not enough so I created such 2 new > tests. > Yes I do think your suggested tests are better, and sync_waituntil should be OK > and async_waituntil should throw, given the description in spec > https://w3c.github.io/ServiceWorker/#dom-fetchevent-respondwith: > ' > Note: event.respondWith(r) extends the lifetime of the event by default as if > event.waitUntil(r) is called. > ' > > I'll revise the tests and try to solve potential bug around them. Thanks! Done. https://codereview.chromium.org/2877543003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:142: // finished, it's hard to get the presise time point between the 2 On 2017/05/25 01:45:02, kinuko wrote: > nit: presise -> precise Done. https://codereview.chromium.org/2877543003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:146: // finished, in such case if there has no any extend lifetime promises are On 2017/05/25 01:45:03, kinuko wrote: > has -> have? > > 'if there aren't any outstanding lifetime-extending promises' ? > > (Is 'extend lifetime promises' a spec term?) Done. Yeah 'extend lifetime promises' is a spec term, keep using this term but changed to 'if there aren't any outstanding extend lifetime promises'
lgtm. Thanks for sticking with this patch. It looks great. I'm glad the new tests caught a bug. https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.cpp (right): https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.cpp:59: // exception' Ah sorry I meant to just link in the codereview. I don't think the spec specifically disallows waitUntil on a script-contructed event. It just falls into the "If the pending promises count is zero and the dispatch flag is unset, then" part. We can just remove this comment. https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:79: // substeps: Decrement the pending promises count by one." add an empty // line after this for easy reading https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:164: "and no any extend lifetime promises are " s/no any/no/ https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h (right): https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:53: // Observes the promise and delays calling the continuation until s/the continuation/reporting to ServiceWorkerGlobalScopeClient that the event completed/ https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:54: // the given promise is resolved or rejected. WaitUntil may be called multiple times. The event is extended until all promises have settled. If provided, |on_promise_fulfilled| or |on_promise_rejected| is invoked once |promise| fulfills or rejects. This enables the caller to do custom handling. https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:57: ScriptPromise, |promise| https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:84: void DecrementPendingPromise(); IncrementPendingPromiseCount DecrementPrendingPromiseCount
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [ServiceWorker] Allow waitUntil to be called multiple times asynchronously BUG=621440 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-async-waituntil.https.html ========== to ========== [ServiceWorker] Allow waitUntil to be called multiple times asynchronously Spec: https://w3c.github.io/ServiceWorker/#dom-extendableevent-waituntil BUG=621440 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-async-waituntil.https.html ==========
Patchset #10 (id:240001) has been deleted
The CQ bit was checked by leon.han@intel.com 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...
The CQ bit was unchecked by leon.han@intel.com
Uploaded ps#10, almost all changes are for code comments, so I'd like to send it to CQ now. Thanks all! https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.cpp (right): https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/ExtendableEvent.cpp:59: // exception' On 2017/05/25 07:23:23, falken wrote: > Ah sorry I meant to just link in the codereview. > > I don't think the spec specifically disallows waitUntil on a script-contructed > event. It just falls into the "If the pending promises count is zero and the > dispatch flag is unset, then" part. > > We can just remove this comment. Oops, got it, removed this comment and updated CL description. Thanks! https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:79: // substeps: Decrement the pending promises count by one." On 2017/05/25 07:23:24, falken wrote: > add an empty // line after this for easy reading Done. https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:164: "and no any extend lifetime promises are " On 2017/05/25 07:23:23, falken wrote: > s/no any/no/ Done. https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h (right): https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:53: // Observes the promise and delays calling the continuation until On 2017/05/25 07:23:24, falken wrote: > s/the continuation/reporting to ServiceWorkerGlobalScopeClient that the event > completed/ Done. https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:54: // the given promise is resolved or rejected. On 2017/05/25 07:23:24, falken wrote: > WaitUntil may be called multiple times. The event is extended until all promises > have settled. > > If provided, |on_promise_fulfilled| or |on_promise_rejected| is invoked once > |promise| fulfills or rejects. This enables the caller to do custom handling. Done. Thanks! Exactly what we should say here ;-) https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:57: ScriptPromise, On 2017/05/25 07:23:24, falken wrote: > |promise| Done. https://codereview.chromium.org/2877543003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:84: void DecrementPendingPromise(); On 2017/05/25 07:23:24, falken wrote: > IncrementPendingPromiseCount > DecrementPrendingPromiseCount Done.
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from shimazu@chromium.org, kinuko@chromium.org, falken@chromium.org Link to the patchset: https://codereview.chromium.org/2877543003/#ps260001 (title: "Address comments from falken@")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1495707115621490,
"parent_rev": "929808a8f063d31a01b883ab8c81c21b838ad61b", "commit_rev":
"eaf348dc9864d8073fe01472a91c33f21e3ec4b6"}
Message was sent while issue was closed.
Description was changed from ========== [ServiceWorker] Allow waitUntil to be called multiple times asynchronously Spec: https://w3c.github.io/ServiceWorker/#dom-extendableevent-waituntil BUG=621440 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-async-waituntil.https.html ========== to ========== [ServiceWorker] Allow waitUntil to be called multiple times asynchronously Spec: https://w3c.github.io/ServiceWorker/#dom-extendableevent-waituntil BUG=621440 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-async-waituntil.https.html Review-Url: https://codereview.chromium.org/2877543003 Cr-Commit-Position: refs/heads/master@{#474630} Committed: https://chromium.googlesource.com/chromium/src/+/eaf348dc9864d8073fe01472a91c... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001) as https://chromium.googlesource.com/chromium/src/+/eaf348dc9864d8073fe01472a91c... |
