[ServiceWorker] Allow waitUntil to be called multiple times asynchronously

third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp

Index: third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp
diff --git a/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp b/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp
index d8056ae69e40194bdf9fdb55b9627aaf8768753c..db14e8694fe75e8b61b25fccf829335350a12870 100644
--- a/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp
+++ b/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp
@@ -12,7 +12,9 @@
 #include "core/dom/ExecutionContext.h"
 #include "modules/serviceworkers/ServiceWorkerGlobalScope.h"
 #include "platform/LayoutTestSupport.h"
+#include "platform/bindings/Microtask.h"
 #include "platform/wtf/Assertions.h"
+#include "platform/wtf/Functional.h"
 #include "public/platform/Platform.h"
 #include "public/platform/modules/serviceworker/WebServiceWorkerEventResult.h"
 #include "v8/include/v8.h"
@@ -69,7 +71,16 @@ class WaitUntilObserver::ThenFunction final : public ScriptFunction {
       value =
           ScriptPromise::Reject(value.GetScriptState(), value).GetScriptValue();
     } else {
-      observer_->OnPromiseFulfilled();
+      // At this time point the microtask A running resolver function of this
+      // promise has already been queued, in order to allow microtask A to call
+      // waitUntil, we enqueue another microtask B to delay the promise resolved
+      // notification, thus A will run before B so A can call waitUntil well,
+      // but any other microtask C possibly enqueued by A will run after B so
+      // C maybe can't call waitUntil if there has no any extend lifetime
+      // promise at that time.
+      Microtask::EnqueueMicrotask(
+          WTF::Bind(&WaitUntilObserver::OnPromiseFulfilled,
+                    WrapPersistent(observer_.Get())));

[leonhsl(Using Gerrit), 2017/05/22 13:23:02]

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...

[shimazu, 2017/05/23 09:08:43]

I think it's good as is because the task should have the ownership of the
observer instead of this ThenFunction. 

[leonhsl(Using Gerrit), 2017/05/23 09:40:18]

Got it. Thanks!

     }
     observer_ = nullptr;
     return value;
@@ -108,9 +119,21 @@ void WaitUntilObserver::DidDispatchEvent(bool event_dispatch_failed) {
 void WaitUntilObserver::WaitUntil(ScriptState* script_state,
                                   ScriptPromise script_promise,
                                   ExceptionState& exception_state) {
-  if (event_dispatch_state_ != EventDispatchState::kInitial) {
+  if (pending_activity_ == 0) {
     exception_state.ThrowDOMException(kInvalidStateError,
-                                      "The event handler is already finished.");
+                                      "The event handler is already finished "
+                                      "and no any extend lifetime promises are "
+                                      "outstanding.");
+    return;
+  }
+
+  if (pending_activity_ == 1 &&
+      event_dispatch_state_ == EventDispatchState::kInitial &&
+      v8::MicrotasksScope::IsRunningMicrotasks(script_state->GetIsolate())) {

[shimazu, 2017/05/22 03:48:41]

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.

[leonhsl(Using Gerrit), 2017/05/22 05:01:43]

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/...

[shimazu, 2017/05/22 08:09:05]

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;
    }
  }
```

[leonhsl(Using Gerrit), 2017/05/22 13:23:02]

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!

+    exception_state.ThrowDOMException(
+        kInvalidStateError,
+        "Although the event handler is not finished yet, it's running "
+        "microtask, and no any extend lifetime promises are outstanding.");
     return;
   }