Bug 28980

Summary:

[V8] Run-time exception in onmessage handler is not forwarded to the worker object.

Product:

WebKit

Reporter:

Jian Li <jianli>

Component:

WebCore Misc.

Assignee:

Jian Li <jianli>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dglazkov, levin, pfeldman, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Proposed Patch	abarth: review+, jianli: commit-queue-
Proposed Patch 2	levin: review+, jianli: commit-queue-

Description Jian Li 2009-09-04 12:10:41 PDT

Run-time exception in onmessage handler is not forwarded to the worker object. Thus the layout test worker-close.html fails in chromium.

Comment 1 Jian Li 2009-09-04 12:21:12 PDT

Created attachment 39083 [details]
Proposed Patch

Comment 2 Adam Barth 2009-09-04 12:26:51 PDT

Comment on attachment 39083 [details]
Proposed Patch

Test?

Also, I don't think getScriptExecutionContext(ScriptState) is correct.  We shouldn't be going through the frame to get the execution context.

Comment 3 Adam Barth 2009-09-04 13:13:18 PDT

Comment on attachment 39083 [details]
Proposed Patch

Jian corrected me that this is already tested by worker-close and that the frame logic is already present.

Comment 4 Jian Li 2009-09-04 15:44:14 PDT

Committed as http://trac.webkit.org/changeset/48072.

Comment 5 Jian Li 2009-09-09 11:46:27 PDT

The commit has been partially reverted due to reliability build break in Chromium. We need to rework on this issue.

Comment 6 Jian Li 2009-09-09 12:01:09 PDT

Created attachment 39289 [details]
Proposed Patch 2

Comment 7 David Levin 2009-09-11 16:13:10 PDT

Comment on attachment 39289 [details]
Proposed Patch 2

Just a few nits to fix on landing.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        The previous fix is partially reverted due to reliability build break

s/is/was/

s/to/to a/

> +        in chromium. The break happens when an exception is thrown without
> +        setting a message. We need to check for this scenario and handle it.

No need to change this but for future reference, in general this type of comment can go with the function where you are doing the check and handling it.

> diff --git a/WebCore/bindings/v8/V8AbstractEventListener.cpp b/WebCore/bindings/v8/V8AbstractEventListener.cpp
> +        tryCatch.SetVerbose(false);   // We do not want to report the exception to the inspector console.

Single space before end of line comment.

Comment 8 Jian Li 2009-09-21 17:09:08 PDT

Committed as http://trac.webkit.org/changeset/48610.

Comment 9 Pavel Feldman 2009-11-03 22:41:23 PST

Guys, could we revert this? At least the verbose part that breaks error reporting for devtools. Btw, why do we introduce handling of exceptions with no messages so late in the cycle? I've not seen those before, or do I miss something?

Comment 10 Adam Barth 2009-11-03 23:01:40 PST

(In reply to comment #9)
> Guys, could we revert this? At least the verbose part that breaks error
> reporting for devtools. Btw, why do we introduce handling of exceptions with no
> messages so late in the cycle? I've not seen those before, or do I miss
> something?

Please file a new bug with this request and cc me, Jian, and levin.

Comment 11 Yury Semikhatsky 2009-11-04 01:06:38 PST

Filed a new bug: https://bugs.webkit.org/show_bug.cgi?id=31108

(In reply to comment #10)
> (In reply to comment #9)
> > Guys, could we revert this? At least the verbose part that breaks error
> > reporting for devtools. Btw, why do we introduce handling of exceptions with no
> > messages so late in the cycle? I've not seen those before, or do I miss
> > something?
> 
> Please file a new bug with this request and cc me, Jian, and levin.