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