Correctly attribute exceptions thrown inside a Worker's imported scripts.

Source/bindings/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 149 matching lines @@
         return ScriptValue();
     }
 
     if (block.HasCaught()) {
         v8::Local<v8::Message> message = block.Message();
         state->hadException = true;
         state->errorMessage = toWebCoreString(message->Get());
         state->lineNumber = message->GetLineNumber();
         state->columnNumber = message->GetStartColumn();
         state->sourceURL = toWebCoreString(message->GetScriptResourceName());
-        if (m_workerGlobalScope->shouldSanitizeScriptError(state->sourceURL, NotSharableCrossOrigin))
-            state->exception = throwError(v8GeneralError, "Script error.", m_isolate);
-        else
-            state->exception = ScriptValue(block.Exception());
+        state->exception = ScriptValue(block.Exception());

[do-not-use, 2013/08/07 18:05:05]

Actually, it looks like we can remove WorkerGlobalScopeExecutionState.exception
now? It no longer seems to be used anywhere.

Sorry, I am not very familiar with this part but isn't it a bad thing that we
"loose" this exception?

[Mike West, 2013/08/08 05:03:15]

In this patch, yes. I plan to follow up with a patch that would make the
exception available as an 'error' property on the ErrorEvent, however, so I'm
not sure it's worth it to drop this line only to read-d it again in a day or
three.
No one's really familiar with this part. I don't think it's been touched since
2010. ;)

I don't think there's any impact to keeping a copy of the exception here. The
WorkerGlobalScopeExecutionState is allocated in
WorkerScriptController::evaluate, and shouldn't persist beyond that. ScriptValue
holds a scoped persistent handle to the exception, and just derefs it when the
object is destructed.

I don't think that will cause any problems.

If it makes you uncomfortable, however, I'd be happy to drop the line until the
next patch actually does something with the exception.

[do-not-use, 2013/08/08 06:25:00]

I think it is fine to keep it if you plan to use it in a follow-up patch soon.

 
         block.Reset();
     } else
         state->hadException = false;
 
     if (result.IsEmpty() || result->IsUndefined())
         return ScriptValue();
 
     return ScriptValue(result);
 }
 
-void WorkerScriptController::evaluate(const ScriptSourceCode& sourceCode, ScriptValue* exception)
+void WorkerScriptController::evaluate(const ScriptSourceCode& sourceCode, RefPtr<ErrorEvent>* errorEvent)
 {
     if (isExecutionForbidden())
         return;
 
     WorkerGlobalScopeExecutionState state;
     evaluate(sourceCode.source(), sourceCode.url().string(), sourceCode.startPosition(), &state);
     if (state.hadException) {
-        if (exception) {
-            *exception = state.exception;
+        if (errorEvent) {
+            *errorEvent = m_workerGlobalScope->shouldSanitizeScriptError(state.sourceURL, NotSharableCrossOrigin) ?
+                ErrorEvent::createSanitizedError() : ErrorEvent::create(state.errorMessage, state.sourceURL, state.lineNumber, state.columnNumber);
         } else {
-            RefPtr<ErrorEvent> event = ErrorEvent::create(state.errorMessage, state.sourceURL, state.lineNumber, state.columnNumber);
+            ASSERT(!m_workerGlobalScope->shouldSanitizeScriptError(state.sourceURL, NotSharableCrossOrigin));
+            RefPtr<ErrorEvent> event = m_errorEventFromImportedScript ? m_errorEventFromImportedScript.release() : ErrorEvent::create(state.errorMessage, state.sourceURL, state.lineNumber, state.columnNumber);
+            m_errorEventFromImportedScript.clear();

[do-not-use, 2013/08/07 18:05:05]

This line looks superfluous since you release() the RefPtr the line before.

[Mike West, 2013/08/08 05:03:15]

Indeed.

             m_workerGlobalScope->reportException(event, 0, 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::setException(const ScriptValue& exception)
+void WorkerScriptController::rethrowExceptionFromImportedScript(PassRefPtr<ErrorEvent> errorEvent)
 {
-    throwError(exception.v8Value());
+    m_errorEventFromImportedScript = errorEvent;
+    throwError(V8ThrowException::createError(v8GeneralError, m_errorEventFromImportedScript->message(), m_isolate));
 }
 
 WorkerScriptController* WorkerScriptController::controllerForContext()
 {
     // Happens on frame destruction, check otherwise GetCurrent() will crash.
     if (!v8::Context::InContext())
         return 0;
     v8::Handle<v8::Context> context = v8::Context::GetCurrent();
     v8::Handle<v8::Object> global = context->Global();
     global = global->FindInstanceInPrototypeChain(V8WorkerGlobalScope::GetTemplate(context->GetIsolate(), WorkerWorld));
     // Return 0 if the current executing context is not the worker context.
     if (global.IsEmpty())
         return 0;
     WorkerGlobalScope* workerGlobalScope = V8WorkerGlobalScope::toNative(global);
     return workerGlobalScope->script();
 }
 
 } // namespace WebCore