[ServiceWorker] Allow waitUntil to be called multiple times asynchronously

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "modules/serviceworkers/WaitUntilObserver.h"
 
 #include "bindings/core/v8/ScriptFunction.h"
 #include "bindings/core/v8/ScriptPromise.h"
 #include "bindings/core/v8/ScriptValue.h"
 #include "bindings/core/v8/V8BindingForCore.h"
 #include "core/dom/ExceptionCode.h"
 #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"
 
 namespace blink {
 
 namespace {
 
 // Timeout before a service worker that was given window interaction
 // permission loses them. The unit is seconds.
@@ skipping 36 matching lines @@
         resolve_type_(type) {}
 
   ScriptValue Call(ScriptValue value) override {
     DCHECK(observer_);
     DCHECK(resolve_type_ == kFulfilled || resolve_type_ == kRejected);
     if (resolve_type_ == kRejected) {
       observer_->OnPromiseRejected();
       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;
   }
 
   Member<WaitUntilObserver> observer_;
   ResolveType resolve_type_;
 };
 
 WaitUntilObserver* WaitUntilObserver::Create(ExecutionContext* context,
@@ skipping 18 matching lines @@
 void WaitUntilObserver::DidDispatchEvent(bool event_dispatch_failed) {
   event_dispatch_state_ = event_dispatch_failed
                               ? EventDispatchState::kFailed
                               : EventDispatchState::kCompleted;
   DecrementPendingActivity();
 }
 
 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;
+  }
+
   if (!execution_context_)
     return;
 
   // When handling a notificationclick event, we want to allow one window to
   // be focused or opened. See comments in ::willDispatchEvent(). When
   // waitUntil() is being used, opening or closing a window must happen in a
   // timeframe specified by windowInteractionTimeout(), otherwise the calls
   // will fail.
   if (type_ == kNotificationClick)
     consume_window_interaction_timer_.StartOneShot(WindowInteractionTimeout(),
@@ skipping 102 matching lines @@
   if (!execution_context_)
     return;
   execution_context_->ConsumeWindowInteraction();
 }
 
 DEFINE_TRACE(WaitUntilObserver) {
   visitor->Trace(execution_context_);
 }
 
 }  // namespace blink