Bug 28622 – Caught exceptions still pause the debugger

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Kevin Lindeman]

If you are debugging JavaScript using the web inspector that would cause an exception to happen and you have pause on exceptions enabled, but it is within a try/catch block, the debugger still pauses on it.

To be more in line with other debuggers (including the Dashcode debugger), the web inspector should not pause on an exception if it is caught.

[Kevin Lindeman]

Created attachment 38390 [details]
proposed patch

This patch gets us the proposed behavior. It waits to see until function exit if an exception still remains - one won't remain if we caught it.

I am not sure what the exception() callback should do if we don't pause anymore.

[Adam Barth]

Comment on attachment 38390 [details]
proposed patch

No ChagneLog.  No test.

[Timothy Hatcher]

This is a good idea. Would be great to land this with a ChangeLog and a manual test in WebCore/manual-tests/inspector.

[Kevin Lindeman]

Alright, I will work on that.

[Brian Weinstein]

Created attachment 46965 [details]
[PATCH] Fix

Don't break on exceptions that are caught in the same function.

[Oliver Hunt]

I disagree, in all debuggers for other languages, pause on exceptions means pause on all exceptions, regardless as to whether there is a handler.  Pause on exceptions is used specifically for the case where you want to pause on an exception they may actually be caught.

[Oliver Hunt]

Rather than looking at things like dashcode, people should be looking at real debuggers such as VS, Eclipse, etc, where (if memory serves) th debugger stops on uncaught exceptions by default, and pause on exceptions actually means pause on all exceptions whether or not there is a handler.

[Timothy Hatcher]

I can se the use of both options.

I think we should have a tri-state button. States being: don't pause on any exceptions, pause on uncaught exceptions, and pause on all exceptions.

[Timothy Hatcher]

Comment on attachment 46965 [details]
[PATCH] Fix

This patch as-is will likely change behaviour in Dashcode or other clients. They might rely on the exeption function always being called.

Since those clients are protected from the C++ layer, I propose we pass another bool to the function: hasHandler.

Then the Debugger can decided if it cares about the exception, and it lets up have a setting. It will also prevent breaking other clients. (You will need to update WebScriptDebugger.mm in WebKit to prevent breaking the build when you add the new argument.)

[Brian Weinstein]

Created attachment 46967 [details]
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception

This patch has no change in behavior, but enables us to differentiate between Break on All Exceptions and Break on Uncaught Exceptions.

[Geoffrey Garen]

Comment on attachment 46967 [details]
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception

I think you need to "UNUSED_PARAM(hasHandler);" in WebScriptDebugger too. The rest of the code looks correct, though.

FWIW, I agree with Oliver about this feature: DashCode's behavior is odd. Maybe we could add a tri-state button. But have any real users requested one?

[Timothy Hatcher]

Comment on attachment 46967 [details]
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception

You don't need UNUSED_PARAM in WebKit.

[Eric Seidel (no email)]

Either this or bug 33820 seems to have broken a webgl test.  At least thats' what the bots say:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r53517%20(9580)/fast/canvas/webgl/texImage2DImageDataTest-pretty-diff.html

The first failure started between r53514 and r53517 inclusive.

[Timothy Hatcher]

I highly doubt it was this change, which has nothing to do with layout or rendering.

[Brian Weinstein]

(In reply to comment #13)
> Either this or bug 33820 seems to have broken a webgl test.  At least thats'
> what the bots say:
> http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r53517%20(9580)/fast/canvas/webgl/texImage2DImageDataTest-pretty-diff.html
> 
> The first failure started between r53514 and r53517 inclusive.

This added a parameter that is unused. It wouldn't have broken any tests I don't think.

[Brian Weinstein]

Comment on attachment 46967 [details]
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception

Landed in r53516.

[Brian Weinstein]

Created attachment 47064 [details]
[PATCH] Tri-State Pause on Exceptions Button

[Timothy Hatcher]

Comment on attachment 47064 [details]
[PATCH] Tri-State Pause on Exceptions Button


> -    bool pauseOnExceptions();
> -    void setPauseOnExceptions(bool pause);
> +    int pauseOnExceptions();
> +    void setPauseOnExceptions(int pause);

I think we should add "State" to these function names. setPauseOnExceptionsState. Why an int here and not long? When the idl uses long.


> -    , m_pauseOnExceptions(false)
> +    , m_pauseOnExceptions(DontPauseOnExceptions)

Add "State" to m_pauseOnExceptions: m_pauseOnExceptionsState.


> +        enum PauseOnExceptionsCondition {
> +            DontPauseOnExceptions,
> +            PauseOnAllExceptions,
> +            PauseOnUncaughtExceptions
> +        };

I think State is better than Condition.

> +        PauseOnExceptionsCondition pauseOnExceptions() const { return m_pauseOnExceptions; }
> +        void setPauseOnExceptions(PauseOnExceptionsCondition);

Add State to the function names.


> +WebInspector.ScriptsPanel.PauseOnExceptionsCondition = {

State.


> +            this.pauseOnExceptionButton.title = WebInspector.UIString("Don't pause on exceptions / Click to Pause on all exceptions.");
> +        else if (InspectorBackend.pauseOnExceptions() == WebInspector.ScriptsPanel.PauseOnExceptionsCondition.PauseOnAllExceptions)
> +            this.pauseOnExceptionButton.title = WebInspector.UIString("Pause on all exceptions  / Click to Pause on uncaught exceptions.");
> +        else if (InspectorBackend.pauseOnExceptions() == WebInspector.ScriptsPanel.PauseOnExceptionsCondition.PauseOnUncaughtExceptions)
> +            this.pauseOnExceptionButton.title = WebInspector.UIString("Pause on uncaught exceptions / Click to Not pause on exceptions.");

Replace the "/" with a \n. I meant to tell you that on IRC.

> +    this.numStates = numStates;
> +    if (!numStates)
> +        this.numStates = 2;

I don't think "num" helps. Just call it "states".

> +    if (numStates == 2)
> +        this._toggled = false;
> +    else
> +        this._toggled = 0;

I think _state would be a better name for _toggled now. 

Overall looks good. Attach another patch if you want me to look over any tweaks.

[Brian Weinstein]

Created attachment 47083 [details]
[PATCH] Tri-State Button + Tim's Comments

This renames the functions to pauseOnExceptionsState, and takes care of other feedback from Tim. Also waiting on feedback from Pavel before landing.

[Timothy Hatcher]

Comment on attachment 47083 [details]
[PATCH] Tri-State Button + Tim's Comments

>      get toggled()
>      set toggled(x)

You should renamed these too. But that will touch more files. Maybe add a get/set state, and make toggled use that only for states == 2? And throw otherwise.

[Brian Weinstein]

Created attachment 47085 [details]
[PATCH] Cleaned up toggled/state Getter/Setter code

Should be a final pass, waiting for Pavel to take a look before landing. Sorry about the numerous iterations.

[Timothy Hatcher]

Comment on attachment 47085 [details]
[PATCH] Cleaned up toggled/state Getter/Setter code

> +            throw("Only used toggled when there are 2 states, otherwise, use state");
> +            throw("Only used toggled when there are 2 states, otherwise, use state");

Typo: "used".

[Brian Weinstein]

(In reply to comment #22)
> (From update of attachment 47085 [details])
> > +            throw("Only used toggled when there are 2 states, otherwise, use state");
> > +            throw("Only used toggled when there are 2 states, otherwise, use state");
> 
> Typo: "used".

Fixed. Thanks!

[Pavel Feldman]

(In reply to comment #21)
> Created an attachment (id=47085) [details]
> [PATCH] Cleaned up toggled/state Getter/Setter code
> 
> Should be a final pass, waiting for Pavel to take a look before landing. Sorry
> about the numerous iterations.

We are now fixing Chromium so that it works with this patch. Will land this one shortly!

[Brian Weinstein]

(In reply to comment #24)
> (In reply to comment #21)
> > Created an attachment (id=47085) [details] [details]
> > [PATCH] Cleaned up toggled/state Getter/Setter code
> > 
> > Should be a final pass, waiting for Pavel to take a look before landing. Sorry
> > about the numerous iterations.
> 
> We are now fixing Chromium so that it works with this patch. Will land this one
> shortly!

Let me know when you land it, I can land my part when it's ready. Thanks for following up on this.

[Yury Semikhatsky]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/JavaScriptDebugServer.cpp
	M	WebCore/inspector/JavaScriptDebugServer.h
	M	WebCore/inspector/front-end/InspectorBackendStub.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	M	WebCore/inspector/front-end/StatusBarButton.js
	M	WebCore/inspector/front-end/inspector.css
Committed r53696