Bug 22310 – Worker exceptions should be printed to console

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Alexey Proskuryakov]

Firefox forwards uncaught worker exceptions to worker.onerror. I'm not sure if this behavior is desirable - for one, the current spec says that this event is just for loading errors. But we certainly should print them to console.

[Alexey Proskuryakov]

Created attachment 25215 [details]
proposed patch

[Darin Adler]

Comment on attachment 25215 [details]
proposed patch

> +            UString errorMessage = exception->toString(exec);
> +            JSObject* exceptionObject = exception->toObject(exec);
> +            int lineNumber = exceptionObject->get(exec, Identifier(exec, "line"))->toInt32(exec);
> +            UString exceptionSourceURL = exceptionObject->get(exec, Identifier(exec, "sourceURL"))->toString(exec);
> +
> +            scriptExecutionContext->reportException(errorMessage, lineNumber, exceptionSourceURL);

This seems like too much code to have here in line. I'd like to see a helper function that does more of this work.

> +            if (exec->hadException())
> +                exec->clearException();

This if seems unnecessary. There's no reason to check for null before setting the value to null.

Also, I think it's OK for new code to get and clear exceptions in the JSGlobalObject without involving ExecState. The API to do it through ExecState is the "old way" in my opinion.

> +        UString errorMessage = exception->toString(exec);
> +        JSObject* exceptionObject = exception->toObject(exec);
> +        int lineNumber = exceptionObject->get(exec, Identifier(exec, "line"))->toInt32(exec);
> +        UString exceptionSourceURL = exceptionObject->get(exec, Identifier(exec, "sourceURL"))->toString(exec);
> +
> +        m_workerContext->thread()->messagingProxy()->postWorkerException(errorMessage, lineNumber, exceptionSourceURL);
> +        exec->clearException();

Here's the same code repeated again. I think this validates my request/suggestion that this be in a helper function.

> +        virtual void reportException(const String& errorMessage, int lineNumber, const String& sourceURL);

Instead of having a default that does nothing, perhaps it would be better to have this be a pure virtual function. The default empty version is only handy if we expect to derive classes that want to leave this out.

r=me as is -- please consider my suggestions.

[Alexey Proskuryakov]

Created attachment 25221 [details]
updated patch

[Alexey Proskuryakov]

Comment on attachment 25215 [details]
proposed patch

Clearing review flag to get the obsoleted patch out of commit queue.

[Darin Adler]

Comment on attachment 25221 [details]
updated patch

> -        Document::updateDocumentsRendering();
> +        if (scriptExecutionContext->isDocument())
> +            Document::updateDocumentsRendering();

Maybe this should be a virtual function on ScriptExecutionContext instead of a hard-coded rule at the callsite?

>  #include "WorkerContext.h"
> +#include "WorkerMessagingProxy.h"
> +#include "WorkerThread.h"
>  #include <parser/SourceCode.h>

Why are you adding these includes to WorkerScriptController.cpp?

r=me

[Alexey Proskuryakov]

Committed revision 38595.

(In reply to comment #5)
> Maybe this should be a virtual function on ScriptExecutionContext instead of a
> hard-coded rule at the callsite?

It looks alien in this function in either case, I'm not sure.

> Why are you adding these includes to WorkerScriptController.cpp?

That's simply a leftover from an earlier iteration - but these includes are now in this file for other reasons!