We need to come up with better design to solve the duplicate error message reporting problem, instead of relying on checking ScriptExecutionContext to avoid duplicate messages.
Created attachment 51150 [details] patch
Comment on attachment 51150 [details] patch This looks sane from the v8 message handling perspective. Could you get lg from the workers guys prior to landing?
Created attachment 51173 [details] patch - Added test - v8MessageHandler is not reentarable otherwise it gets into an infinite recursion if there is an exception in onerror handler
> diff --git a/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp b/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp > +static void v8MessageHandler(v8::Handle<v8::Message> message, v8::Handle<v8::Value> data) > +{ > + static bool isReportingException = false; Do we need to say "static bool" since we are already in a static function? > + // Exceptions that occur in error handler should be ignored since in that case An extra space after "since". > + // WorkerContext::reportException will send the exception to the worker object. > + if (isReportingException) > + return; > + isReportingException = true; > + > + String errorMessage = toWebCoreString(message->Get()); Do we want to check if the message is empty? Previously we got a reliability build break in chromium due to that the exception can be thrown without setting a message.
(In reply to comment #4) > > diff --git a/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp b/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp > > +static void v8MessageHandler(v8::Handle<v8::Message> message, v8::Handle<v8::Value> data) > > +{ > > + static bool isReportingException = false; > Do we need to say "static bool" since we are already in a static function? Yes, because we want it to be a static variable. > > + // Exceptions that occur in error handler should be ignored since in that case > An extra space after "since". Fixed. > > + // WorkerContext::reportException will send the exception to the worker object. > > + if (isReportingException) > > + return; > > + isReportingException = true; > > + > > + String errorMessage = toWebCoreString(message->Get()); > Do we want to check if the message is empty? Previously we got a reliability > build break in chromium due to that the exception can be thrown without setting > a message. Previously the message was pulled from v8::TryCatch that has SetCaptureMessage method that allows to disable message capturing. In MessageHandler add with v8::AddMessageHandler the message handle can be empty only if the exception was thrown when v8 bootstrapper is active which should never happen in normal conditions that's why there is no IsEmpty() check in V8ConsoleMessage::hanler where uncaught exceptions from Page are reporte.d
Created attachment 51271 [details] patch
Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/fast/workers/resources/worker-exception-in-timeout-callback.js M LayoutTests/fast/workers/worker-script-error-expected.txt M LayoutTests/fast/workers/worker-script-error.html M WebCore/ChangeLog M WebCore/bindings/v8/V8AbstractEventListener.cpp M WebCore/bindings/v8/V8Utilities.cpp M WebCore/bindings/v8/V8Utilities.h M WebCore/bindings/v8/WorkerContextExecutionProxy.cpp Committed r56329
Comment on attachment 51150 [details] patch Cleared Pavel Feldman's review+ from obsolete attachment 51150 [details] so that this bug does not appear in http://webkit.org/pending-commit.