Propagate nested importScripts() error events outwards.

Source/bindings/core/v8/WorkerScriptController.cpp

Index: Source/bindings/core/v8/WorkerScriptController.cpp
diff --git a/Source/bindings/core/v8/WorkerScriptController.cpp b/Source/bindings/core/v8/WorkerScriptController.cpp
index d7e3a93b43fd4276e6d5dec0044caf869bb75833..99c77a91b84bb4ef8420757167fd9aa1883aa93e 100644
--- a/Source/bindings/core/v8/WorkerScriptController.cpp
+++ b/Source/bindings/core/v8/WorkerScriptController.cpp
@@ -58,11 +58,47 @@
 
 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);
@@ -153,7 +189,7 @@ bool WorkerScriptController::initializeContextIfNeeded()
     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();
@@ -179,16 +215,16 @@ ScriptValue WorkerScriptController::evaluate(const String& script, const String&
 
     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())
@@ -202,21 +238,27 @@ void WorkerScriptController::evaluate(const ScriptSourceCode& sourceCode, RefPtr
     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);
         }
     }
@@ -258,10 +300,12 @@ 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