Make messageHandlerInMainThread support main-thread worklets.

third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp

Index: third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp b/third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp
index 2aae5b22c7a59fd33cd33083d1b7ff233abc770f..227af320263ec45ad53756abf2f6a927ba2fa908 100644
--- a/third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp
@@ -111,11 +111,11 @@ static PassRefPtr<ScriptCallStack> extractCallStack(v8::Isolate* isolate, v8::Lo
     return callStack.release();
 }
 
-static String extractResourceName(v8::Local<v8::Message> message, const Document* document)
+static String extractResourceName(v8::Local<v8::Message> message, const ExecutionContext* context)
 {
     v8::Local<v8::Value> resourceName = message->GetScriptOrigin().ResourceName();
-    bool shouldUseDocumentURL = document && (resourceName.IsEmpty() || !resourceName->IsString());
-    return shouldUseDocumentURL ? document->url() : toCoreString(resourceName.As<v8::String>());
+    bool shouldUseDocumentURL = context->isDocument() && (resourceName.IsEmpty() || !resourceName->IsString());
+    return shouldUseDocumentURL ? context->url() : toCoreString(resourceName.As<v8::String>());
 }
 
 static String extractMessageForConsole(v8::Isolate* isolate, v8::Local<v8::Value> data)
@@ -147,10 +147,10 @@ static void messageHandlerInMainThread(v8::Local<v8::Message> message, v8::Local
 {
     ASSERT(isMainThread());
     v8::Isolate* isolate = v8::Isolate::GetCurrent();
-    // If called during context initialization, there will be no entered window.
-    // TODO(haraken): Add a helper method to get an entered window that may be null.
-    LocalDOMWindow* enteredWindow = toLocalDOMWindow(toDOMWindow(isolate->GetEnteredContext()));
-    if (!enteredWindow || !enteredWindow->isCurrentlyDisplayedInFrame())
+
+    // If called during context initialization, there will be no entered context.
+    ScriptState* scriptState = ScriptState::current(isolate);
+    if (!scriptState->contextIsValid())
         return;
 
     int scriptId = 0;
@@ -162,9 +162,8 @@ static void messageHandlerInMainThread(v8::Local<v8::Message> message, v8::Local
     else if (message->IsSharedCrossOrigin())
         accessControlStatus = SharableCrossOrigin;
 
-    ScriptState* scriptState = ScriptState::current(isolate);
-
-    String resourceName = extractResourceName(message, enteredWindow->document());
+    ExecutionContext* context = scriptState->getExecutionContext();
+    String resourceName = extractResourceName(message, context);
     ErrorEvent* event = createErrorEventFromMesssage(scriptState, message, resourceName);
 
     String messageForConsole = extractMessageForConsole(isolate, data);
@@ -174,9 +173,11 @@ static void messageHandlerInMainThread(v8::Local<v8::Message> message, v8::Local
     // This method might be called while we're creating a new context. In this case, we
     // avoid storing the exception object, as we can't create a wrapper during context creation.
     // FIXME: Can we even get here during initialization now that we bail out when GetEntered returns an empty handle?
-    LocalFrame* frame = enteredWindow->document()->frame();
-    if (frame && frame->script().existingWindowProxy(scriptState->world())) {
-        V8ErrorHandler::storeExceptionOnErrorEventWrapper(scriptState, event, data, scriptState->context()->Global());
+    if (context->isDocument()) {
+        LocalFrame* frame = toDocument(context)->frame();
+        if (frame && frame->script().existingWindowProxy(scriptState->world())) {
+            V8ErrorHandler::storeExceptionOnErrorEventWrapper(scriptState, event, data, scriptState->context()->Global());
+        }
     }
 
     if (scriptState->world().isPrivateScriptIsolatedWorld()) {
@@ -187,9 +188,9 @@ static void messageHandlerInMainThread(v8::Local<v8::Message> message, v8::Local
         // other isolated worlds (which means that the error events won't fire any event listeners
         // in user's scripts).
         EventDispatchForbiddenScope::AllowUserAgentEvents allowUserAgentEvents;
-        enteredWindow->document()->reportException(event, scriptId, callStack, accessControlStatus);
+        context->reportException(event, scriptId, callStack, accessControlStatus);
     } else {
-        enteredWindow->document()->reportException(event, scriptId, callStack, accessControlStatus);
+        context->reportException(event, scriptId, callStack, accessControlStatus);
     }
 }
 
@@ -270,13 +271,19 @@ static void promiseRejectHandlerInMainThread(v8::PromiseRejectMessage data)
     v8::Local<v8::Promise> promise = data.GetPromise();
 
     v8::Isolate* isolate = promise->GetIsolate();
-    // There is no entered window during microtask callbacks from V8,
-    // thus we call toDOMWindow() instead of enteredDOMWindow().
+
+    // This check is used by extensions ModuleSystemTest.
     LocalDOMWindow* window = currentDOMWindow(isolate);

[ikilpatrick, 2016/04/13 23:26:53]

There is a series of extension tests that require this check, see patch set 8
test results:
https://luci-logdog.appspot.com/v/?s=bb%2Ftryserver.chromium.linux%2Flinux_ch...

Basically the tests fail on creating a newLocal of the context:
#3 0x000004d6b13e blink::ScopedPersistent<>::newLocal()
#4 0x000004d68543 blink::ScriptState::context()
#5 0x000004d6b571 blink::ScriptState::from()
#6 0x000004ef0ba6 blink::promiseRejectHandlerInMainThread()

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

How can a newLocal can fail?

[haraken, 2016/04/14 02:02:00]

This would be caused because promiseRejectHandlerInMainThread() is called when
there is no valid ScriptState on the top of the stack. This needs to be fixed
but would be a separate issue.

Can we add:

  // TODO: Remove the check.
  if (!ScriptState::hasCurrentScriptState(isolate))
    return;

?

[ikilpatrick, 2016/04/14 04:09:53]

I tried this and doesn't appear to fix it. Thoughts?

[haraken, 2016/04/14 04:13:17]

OK, then let's keep the current code as is with a TODO.

I think a right fix would be to make the to-be-rejected promise hold a
ScriptState, or something.

+yhirano

[ikilpatrick, 2016/04/14 04:22:51]

Sounds good.

I updated the comment to:

    // TODO(ikilpatrick): Remove this check, extensions tests that use
    // extensions::ModuleSystemTest incorrectly don't have a valid script state.

     if (!window || !window->isCurrentlyDisplayedInFrame())
         return;
 
-    promiseRejectHandler(data, rejectedPromisesOnMainThread(), window->document() ? window->document()->url() : String());
+    // Bail out if called during context initialization.
+    ScriptState* scriptState = ScriptState::current(isolate);
+    if (!scriptState->contextIsValid())
+        return;
+
+    ExecutionContext* executionContext = scriptState->getExecutionContext();
+    promiseRejectHandler(data, rejectedPromisesOnMainThread(), executionContext ? executionContext->url() : String());

[ikilpatrick, 2016/04/13 23:26:53]

The nullptr check for executionContext->url() is still needed as
ScriptPromiseTest fails without it.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

It is using a TestingScriptState that doesn't have an associated
ExecutionContext. In real code can that ever happen? I.e. a ScriptState not
having an associated ExecutionContext?

(if it can we'll need to add back in a few nullptr checks on executioncontext).

[haraken, 2016/04/14 02:02:00]

I don't know why the ScriptPromiseTest fails here.  ScriptStateForTesting
guarantees that scriptState->getExecutionContext() returns a valid
ExecutionContext, doesn't it?

[ikilpatrick, 2016/04/14 04:09:53]

Dug into this a bit, ScriptStateForTesting explicitly needs an ExecutionContext
set. Mosts tests do this, this one doesn't, I've modified the test.

 }
 
 static void promiseRejectHandlerInWorker(v8::PromiseRejectMessage data)
@@ -425,9 +432,9 @@ static void messageHandlerInWorker(v8::Local<v8::Message> message, v8::Local<v8:
         return;
     perIsolateData->setReportingException(true);
 
-    ScriptState* scriptState = ScriptState::current(isolate);
     // During the frame teardown, there may not be a valid context.
-    if (ExecutionContext* context = scriptState->getExecutionContext()) {
+    ScriptState* scriptState = ScriptState::current(isolate);
+    if (scriptState->contextIsValid()) {

[haraken, 2016/04/14 02:02:00]

I'd move the line 436 and 437 to just after line 427. We can early-return if
scriptState->contextIsValid() returns false.

[ikilpatrick, 2016/04/14 04:09:53]

Done.

         TOSTRING_VOID(V8StringResource<>, resourceName, message->GetScriptOrigin().ResourceName());
         ErrorEvent* event = createErrorEventFromMesssage(scriptState, message, resourceName);
 
@@ -440,7 +447,7 @@ static void messageHandlerInWorker(v8::Local<v8::Message> message, v8::Local<v8:
         // the error event from the v8::Message, quietly leave.
         if (!isolate->IsExecutionTerminating()) {
             V8ErrorHandler::storeExceptionOnErrorEventWrapper(scriptState, event, data, scriptState->context()->Global());
-            context->reportException(event, scriptId, callStack, corsStatus);
+            scriptState->getExecutionContext()->reportException(event, scriptId, callStack, corsStatus);
         }
     }