Bug 31108 – [V8] REGRESSION: Pause on exception is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yury Semikhatsky]

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

[Yury Semikhatsky]

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).

[Adam Barth]

(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?

[Pavel Feldman]

(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.

[Dimitri Glazkov (Google)]

Looks like if jianli broke it, he should be the one fixing this, right?

[Jian Li]

Created attachment 42501 [details]
Proposed Patch

[Pavel Feldman]

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.

[Yury Semikhatsky]

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

[Yury Semikhatsky]

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?

[Jian Li]

Created attachment 42526 [details]
Proposed Patch

Fixed the problem that duplicate error messages could be shown in the console.

[Yury Semikhatsky]

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.

[Pavel Feldman]

Comment on attachment 42526 [details]
Proposed Patch

r+ given FIXME is added and bug is filed.

[Jian Li]

Bug filed: https://bugs.webkit.org/show_bug.cgi?id=31171

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