Bug 31171 – [V8] Need better design to solve the duplicate error message reporting problem.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jian Li]

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.

[Yury Semikhatsky]

Created attachment 51150 [details]
patch

[Pavel Feldman]

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?

[Yury Semikhatsky]

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

[Jian Li]

> 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.

[Yury Semikhatsky]

(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

[Yury Semikhatsky]

Created attachment 51271 [details]
patch

[Yury Semikhatsky]

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

[Eric Seidel (no email)]

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.