Bug 31108 - [V8] REGRESSION: Pause on exception is broken
Summary: [V8] REGRESSION: Pause on exception is broken
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-11-04 01:05 PST by Yury Semikhatsky
Modified: 2009-11-05 10:36 PST (History)
5 users (show)

See Also:


Attachments
A test page (222 bytes, text/html)
2009-11-04 01:05 PST, Yury Semikhatsky
no flags Details
Proposed Patch (1.54 KB, patch)
2009-11-04 11:20 PST, Jian Li
pfeldman: review+
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (2.45 KB, patch)
2009-11-04 14:58 PST, Jian Li
pfeldman: 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 Yury Semikhatsky 2009-11-04 01:05:33 PST
Created attachment 42462 [details]
A test page

What steps will reproduce the problem?
1. Open the attached page.
2. Open devtools and make sure 'Pause on exception' is on(by default it is).
3. Click 'Test' button.

What is the expected output?
Execution is paused inside f();

What do you see instead?
Exception is recorded in console but script execution is not paused in the Scripts panel and you cannot inspect the call stack.

This is a regression introduced by http://trac.webkit.org/changeset/48610
Comment 1 Yury Semikhatsky 2009-11-04 06:40:14 PST
I think we should change MessageCallback added to v8 by V8Proxy. Currently the callback is V8ConsoleMessage::handler but it seems that it just needs to dispatch  the exception message to the right handler(page's document or worker context).
Comment 2 Adam Barth 2009-11-04 09:14:05 PST
(In reply to comment #1)
> I think we should change MessageCallback added to v8 by V8Proxy. Currently the
> callback is V8ConsoleMessage::handler but it seems that it just needs to
> dispatch  the exception message to the right handler(page's document or worker
> context).

I'm not too familiar with how the devtools hook in here.  Can you upload a patch that does the above so we can see what it looks like in code?
Comment 3 Pavel Feldman 2009-11-04 09:33:34 PST
(In reply to comment #2)
> (In reply to comment #1)
> > I think we should change MessageCallback added to v8 by V8Proxy. Currently the
> > callback is V8ConsoleMessage::handler but it seems that it just needs to
> > dispatch  the exception message to the right handler(page's document or worker
> > context).
> 
> I'm not too familiar with how the devtools hook in here.  Can you upload a
> patch that does the above so we can see what it looks like in code?

We bisected regression to this change and it does not seem to justify well enough why it decided to temporarily set verbose to false in this very place of the code.

The suggested fix is to basically revert trunk/WebCore/bindings/v8/V8AbstractEventListener.cpp change introduced in http://trac.webkit.org/changeset/48610. In fact that is why comment was initially a follow up to that bug.

Note that there is jam's change that happened in between, but i think it should stay.
Comment 4 Dimitri Glazkov (Google) 2009-11-04 09:36:01 PST
Looks like if jianli broke it, he should be the one fixing this, right?
Comment 5 Jian Li 2009-11-04 11:20:56 PST
Created attachment 42501 [details]
Proposed Patch
Comment 6 Pavel Feldman 2009-11-04 11:58:32 PST
Comment on attachment 42501 [details]
Proposed Patch

Thanks for fixing this!
We should make "stop on exceptions" work now and worry about dupe logs after.
Please submit a bug if you come across any.
Comment 7 Yury Semikhatsky 2009-11-04 12:57:51 PST
This patch will lead to duplicated messages in console. If there is an exception in the handler it will be first reported to the console because the TryCatch is verbose and then it will be reported in reportException.

(In reply to comment #5)
> Created an attachment (id=42501) [details]
> Proposed Patch
Comment 8 Yury Semikhatsky 2009-11-04 13:01:10 PST
Yes, I started to work on the change but we need to prepare for GDD now. I'll send it later.

(In reply to comment #2)
> I'm not too familiar with how the devtools hook in here.  Can you upload a
> patch that does the above so we can see what it looks like in code?
Comment 9 Jian Li 2009-11-04 14:58:49 PST
Created attachment 42526 [details]
Proposed Patch

Fixed the problem that duplicate error messages could be shown in the console.
Comment 10 Yury Semikhatsky 2009-11-04 23:06:38 PST
This patch seems to fix the issue with console duplicates. But please add a FIXME comment and file a bug for this. We should not discriminate between different ScriptExecutionContext types when reporting exceptions.

(In reply to comment #9)
> Created an attachment (id=42526) [details]
> Proposed Patch
> 
> Fixed the problem that duplicate error messages could be shown in the console.
Comment 11 Pavel Feldman 2009-11-04 23:09:42 PST
Comment on attachment 42526 [details]
Proposed Patch

r+ given FIXME is added and bug is filed.
Comment 12 Jian Li 2009-11-05 10:36:03 PST
Bug filed: https://bugs.webkit.org/show_bug.cgi?id=31171

FIXME added and committed as http://trac.webkit.org/changeset/50568.