Bug 28622 - Caught exceptions still pause the debugger
: Caught exceptions still pause the debugger
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-08-21 13:13 PST by
Modified: 2010-01-22 04:53 PST (History)


Attachments
proposed patch (1.42 KB, patch)
2009-08-21 13:18 PST, Kevin Lindeman
abarth: review-
Review Patch | Details | Formatted Diff | Diff
[PATCH] Fix (4.22 KB, patch)
2010-01-19 17:29 PST, Brian Weinstein
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception (7.70 KB, patch)
2010-01-19 18:49 PST, Brian Weinstein
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Tri-State Pause on Exceptions Button (58.99 KB, patch)
2010-01-20 14:47 PST, Brian Weinstein
timothy: review+
bweinstein: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
[PATCH] Tri-State Button + Tim's Comments (59.48 KB, patch)
2010-01-20 17:15 PST, Brian Weinstein
timothy: review+
bweinstein: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
[PATCH] Cleaned up toggled/state Getter/Setter code (59.96 KB, patch)
2010-01-20 17:40 PST, Brian Weinstein
timothy: review+
bweinstein: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-08-21 13:13:21 PST
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.
------- Comment #1 From 2009-08-21 13:18:55 PST -------
Created an attachment (id=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.
------- Comment #2 From 2009-09-01 16:27:02 PST -------
(From update of attachment 38390 [details])
No ChagneLog.  No test.
------- Comment #3 From 2009-09-11 20:27:29 PST -------
This is a good idea. Would be great to land this with a ChangeLog and a manual test in WebCore/manual-tests/inspector.
------- Comment #4 From 2009-09-12 16:26:15 PST -------
Alright, I will work on that.
------- Comment #5 From 2010-01-19 17:29:24 PST -------
Created an attachment (id=46965) [details]
[PATCH] Fix

Don't break on exceptions that are caught in the same function.
------- Comment #6 From 2010-01-19 17:32:42 PST -------
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.
------- Comment #7 From 2010-01-19 17:34:03 PST -------
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.
------- Comment #8 From 2010-01-19 17:40:33 PST -------
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.
------- Comment #9 From 2010-01-19 17:45:35 PST -------
(From update of attachment 46965 [details])
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.)
------- Comment #10 From 2010-01-19 18:49:21 PST -------
Created an attachment (id=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.
------- Comment #11 From 2010-01-19 18:56:38 PST -------
(From update of attachment 46967 [details])
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?
------- Comment #12 From 2010-01-19 18:58:41 PST -------
(From update of attachment 46967 [details])
You don't need UNUSED_PARAM in WebKit.
------- Comment #13 From 2010-01-19 23:40:58 PST -------
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.
------- Comment #14 From 2010-01-19 23:54:02 PST -------
I highly doubt it was this change, which has nothing to do with layout or rendering.
------- Comment #15 From 2010-01-19 23:58:42 PST -------
(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.
------- Comment #16 From 2010-01-20 00:06:35 PST -------
(From update of attachment 46967 [details])
Landed in r53516.
------- Comment #17 From 2010-01-20 14:47:34 PST -------
Created an attachment (id=47064) [details]
[PATCH] Tri-State Pause on Exceptions Button
------- Comment #18 From 2010-01-20 16:39:29 PST -------
(From update of attachment 47064 [details])

> -    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.
------- Comment #19 From 2010-01-20 17:15:31 PST -------
Created an attachment (id=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.
------- Comment #20 From 2010-01-20 17:19:33 PST -------
(From update of attachment 47083 [details])
>      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.
------- Comment #21 From 2010-01-20 17:40:17 PST -------
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.
------- Comment #22 From 2010-01-20 17:42:07 PST -------
(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".
------- Comment #23 From 2010-01-20 17:43:27 PST -------
(In reply to comment #22)
> (From update of attachment 47085 [details] [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!
------- Comment #24 From 2010-01-20 22:26:11 PST -------
(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!
------- Comment #25 From 2010-01-20 22:59:08 PST -------
(In reply to comment #24)
> (In reply to comment #21)
> > Created an attachment (id=47085) [details] [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.
------- Comment #26 From 2010-01-22 04:53:18 PST -------
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