Propagate nested importScripts() error events outwards.

Source/bindings/core/v8/WorkerScriptController.cpp

 /*
  * Copyright (C) 2009, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 40 matching lines @@
 #include "core/workers/WorkerGlobalScope.h"
 #include "core/workers/WorkerObjectProxy.h"
 #include "core/workers/WorkerThread.h"
 #include "platform/heap/ThreadState.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebWorkerRunLoop.h"
 #include <v8.h>
 
 namespace WebCore {
 
+class WorkerScriptController::WorkerGlobalScopeExecutionState FINAL {
+    DISALLOW_ALLOCATION();

[haraken, 2014/07/18 01:12:38]

Can we use STACK_ALLOCATED()?

I agree that this object is stack-allocated, but another way would be using
NoBaseWillBeGarbageCollected. Then we can make
WorkerGlobalScopeExecutionState::m_outerState a Member and
WorkerScriptController::m_globalScopeExecutionState a Persistent, which will
make the lifetime relationship clearer.

I don't have a strong opinion on this though.

[sof, 2014/07/18 07:00:46]

The use of STACK_ALLOCATED() makes the GC plugin complain that the off-heap
WorkerScriptController has a field with a stack allocation. I don't see the
problem with that, at least not here.

Moving WorkerGlobalScopeExecutionState onto the heap (along with keeping a
Persistent) is how to make the GC plugin content, as you suggest, but that
strikes me as a workaround. These objects have a clear stack lifetime and the
stack is there and will be scanned should a GC happen. Are we leaving ourselves
open to vulnerabilities by not doing so?

[haraken, 2014/07/18 07:15:34]

Makes sense.

I don't think it's a bug of the plugin. The plugin is just complaining that the
WorkerScriptController has a raw pointer to a stack-allocated object. This is
not safe in general, so the complaint makes sense.

Probably a right fix would be:

- Make WorkerGlobalScopeExecutionState STACK_ALLOCATED().

- Add IGNORE_GC_PLUGIN() to the raw pointers with a comment.

What do you think?

[sof, 2014/07/18 07:36:17]

I think that is an improvement, the natural annotation for it is
STACK_ALLOCATED(). Added now.

+public:
+    explicit WorkerGlobalScopeExecutionState(WorkerScriptController* controller)
+        : hadException(false)
+        , lineNumber(0)
+        , columnNumber(0)
+        , m_controller(controller)
+        , m_outerState(controller->m_globalScopeExecutionState)
+    {
+        m_controller->m_globalScopeExecutionState = this;
+    }
+
+    ~WorkerGlobalScopeExecutionState()
+    {
+        m_controller->m_globalScopeExecutionState = m_outerState;
+    }
+
+    void trace(Visitor* visitor)
+    {
+        visitor->trace(m_errorEventFromImportedScript);
+    }
+
+    bool hadException;
+    String errorMessage;
+    int lineNumber;
+    int columnNumber;
+    String sourceURL;
+    ScriptValue exception;
+    RefPtrWillBeMember<ErrorEvent> m_errorEventFromImportedScript;
+
+    WorkerScriptController* m_controller;
+    WorkerGlobalScopeExecutionState* m_outerState;

[haraken, 2014/07/18 01:12:38]

Shall we add a comment about why we don't need to trace this?

[sof, 2014/07/18 07:00:46]

Yes, these two deserve some explanation :)

+};
+
 WorkerScriptController::WorkerScriptController(WorkerGlobalScope& workerGlobalScope)
     : m_isolate(v8::Isolate::New())
     , m_workerGlobalScope(workerGlobalScope)
     , m_executionForbidden(false)
     , m_executionScheduledToTerminate(false)
+    , m_globalScopeExecutionState(0)
 {
     m_isolate->Enter();
     V8Initializer::initializeWorker(m_isolate);
     v8::V8::Initialize();
     V8PerIsolateData::ensureInitialized(m_isolate);
     m_world = DOMWrapperWorld::create(WorkerWorldId);
     m_interruptor = adoptPtr(new V8IsolateInterruptor(m_isolate));
     ThreadState::current()->addInterruptor(m_interruptor.get());
 }
 
@@ skipping 70 matching lines @@
 
     V8DOMWrapper::associateObjectWithWrapper<V8WorkerGlobalScope>(PassRefPtrWillBeRawPtr<WorkerGlobalScope>(&m_workerGlobalScope), contextType, jsWorkerGlobalScope, m_isolate, WrapperConfiguration::Dependent);
 
     // Insert the object instance as the prototype of the shadow object.
     v8::Handle<v8::Object> globalObject = v8::Handle<v8::Object>::Cast(m_scriptState->context()->Global()->GetPrototype());
     globalObject->SetPrototype(jsWorkerGlobalScope);
 
     return true;
 }
 
-ScriptValue WorkerScriptController::evaluate(const String& script, const String& fileName, const TextPosition& scriptStartPosition, WorkerGlobalScopeExecutionState* state)
+ScriptValue WorkerScriptController::evaluate(const String& script, const String& fileName, const TextPosition& scriptStartPosition)
 {
     if (!initializeContextIfNeeded())
         return ScriptValue();
 
     ScriptState::Scope scope(m_scriptState.get());
 
     if (!m_disableEvalPending.isEmpty()) {
         m_scriptState->context()->AllowCodeGenerationFromStrings(false);
         m_scriptState->context()->SetErrorMessageForCodeGenerationFromStrings(v8String(m_isolate, m_disableEvalPending));
         m_disableEvalPending = String();
     }
 
     v8::TryCatch block;
 
     v8::Handle<v8::String> scriptString = v8String(m_isolate, script);
     v8::Handle<v8::Script> compiledScript = V8ScriptRunner::compileScript(scriptString, fileName, scriptStartPosition, 0, m_isolate);
     v8::Local<v8::Value> result = V8ScriptRunner::runCompiledScript(compiledScript, &m_workerGlobalScope, m_isolate);
 
     if (!block.CanContinue()) {
         m_workerGlobalScope.script()->forbidExecution();
         return ScriptValue();
     }
 
     if (block.HasCaught()) {
         v8::Local<v8::Message> message = block.Message();
-        state->hadException = true;
-        state->errorMessage = toCoreString(message->Get());
-        state->lineNumber = message->GetLineNumber();
-        state->columnNumber = message->GetStartColumn() + 1;
+        m_globalScopeExecutionState->hadException = true;
+        m_globalScopeExecutionState->errorMessage = toCoreString(message->Get());
+        m_globalScopeExecutionState->lineNumber = message->GetLineNumber();
+        m_globalScopeExecutionState->columnNumber = message->GetStartColumn() + 1;
         TOSTRING_DEFAULT(V8StringResource<>, sourceURL, message->GetScriptOrigin().ResourceName(), ScriptValue());
-        state->sourceURL = sourceURL;
-        state->exception = ScriptValue(m_scriptState.get(), block.Exception());
+        m_globalScopeExecutionState->sourceURL = sourceURL;
+        m_globalScopeExecutionState->exception = ScriptValue(m_scriptState.get(), block.Exception());
         block.Reset();
     } else {
-        state->hadException = false;
+        m_globalScopeExecutionState->hadException = false;
     }
 
     if (result.IsEmpty() || result->IsUndefined())
         return ScriptValue();
 
     return ScriptValue(m_scriptState.get(), result);
 }
 
 void WorkerScriptController::evaluate(const ScriptSourceCode& sourceCode, RefPtrWillBeRawPtr<ErrorEvent>* errorEvent)
 {
     if (isExecutionForbidden())
         return;
 
-    WorkerGlobalScopeExecutionState state;
-    evaluate(sourceCode.source(), sourceCode.url().string(), sourceCode.startPosition(), &state);
+    WorkerGlobalScopeExecutionState state(this);
+    evaluate(sourceCode.source(), sourceCode.url().string(), sourceCode.startPosition());
     if (state.hadException) {
         if (errorEvent) {
-            *errorEvent = m_workerGlobalScope.shouldSanitizeScriptError(state.sourceURL, NotSharableCrossOrigin) ?
-                ErrorEvent::createSanitizedError(m_world.get()) : ErrorEvent::create(state.errorMessage, state.sourceURL, state.lineNumber, state.columnNumber, m_world.get());
+            if (state.m_errorEventFromImportedScript) {
+                // Propagate inner error event outwards.
+                *errorEvent = state.m_errorEventFromImportedScript.release();
+                return;
+            }
+            if (m_workerGlobalScope.shouldSanitizeScriptError(state.sourceURL, NotSharableCrossOrigin))
+                *errorEvent = ErrorEvent::createSanitizedError(m_world.get());
+            else
+                *errorEvent = ErrorEvent::create(state.errorMessage, state.sourceURL, state.lineNumber, state.columnNumber, m_world.get());
             V8ErrorHandler::storeExceptionOnErrorEventWrapper(errorEvent->get(), state.exception.v8Value(), m_scriptState->context()->Global(), m_isolate);
         } else {
             ASSERT(!m_workerGlobalScope.shouldSanitizeScriptError(state.sourceURL, NotSharableCrossOrigin));
             RefPtrWillBeRawPtr<ErrorEvent> event = nullptr;
-            if (m_errorEventFromImportedScript) {
-                event = m_errorEventFromImportedScript.release();
-            } else {
+            if (state.m_errorEventFromImportedScript)
+                event = state.m_errorEventFromImportedScript.release();
+            else
                 event = ErrorEvent::create(state.errorMessage, state.sourceURL, state.lineNumber, state.columnNumber, m_world.get());
-            }
             m_workerGlobalScope.reportException(event, nullptr, NotSharableCrossOrigin);
         }
     }
 }
 
 void WorkerScriptController::scheduleExecutionTermination()
 {
     // The mutex provides a memory barrier to ensure that once
     // termination is scheduled, isExecutionTerminating will
     // accurately reflect that state when called from another thread.
@@ skipping 21 matching lines @@
 {
     ASSERT(m_workerGlobalScope.isContextThread());
     return m_executionForbidden;
 }
 
 void WorkerScriptController::disableEval(const String& errorMessage)
 {
     m_disableEvalPending = errorMessage;
 }
 
-void WorkerScriptController::rethrowExceptionFromImportedScript(PassRefPtrWillBeRawPtr<ErrorEvent> errorEvent)
+void WorkerScriptController::rethrowExceptionFromImportedScript(PassRefPtrWillBeRawPtr<ErrorEvent> errorEvent, ExceptionState& exceptionState)
 {
-    m_errorEventFromImportedScript = errorEvent;
-    throwError(V8ThrowException::createError(v8GeneralError, m_errorEventFromImportedScript->message(), m_isolate), m_isolate);
+    const String& errorMessage = errorEvent->message();
+    if (m_globalScopeExecutionState)
+        m_globalScopeExecutionState->m_errorEventFromImportedScript = errorEvent;
+    exceptionState.rethrowV8Exception(V8ThrowException::createError(v8GeneralError, errorMessage, m_isolate));
 }
 
 } // namespace WebCore