Bug 28980 - [V8] Run-time exception in onmessage handler is not forwarded to the worker object.
Summary: [V8] Run-time exception in onmessage handler is not forwarded to the worker o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-04 12:10 PDT by Jian Li
Modified: 2009-11-04 01:06 PST (History)
5 users (show)

See Also:


Attachments
Proposed Patch (12.35 KB, patch)
2009-09-04 12:21 PDT, Jian Li
abarth: review+
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch 2 (3.19 KB, patch)
2009-09-09 12:01 PDT, Jian Li
levin: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.