Bug 129655 - Inspector does not restore breakpoints after a page reload
Summary: Inspector does not restore breakpoints after a page reload
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-03 20:11 PST by Mark Lam
Modified: 2014-03-04 20:38 PST (History)
7 users (show)

See Also:


Attachments
reproduction test case (290 bytes, application/x-gzip)
2014-03-03 20:11 PST, Mark Lam
no flags Details
the patch (7.36 KB, patch)
2014-03-04 16:29 PST, BJ Burg
no flags Details | Formatted Diff | Diff
the patch (revised) (7.26 KB, patch)
2014-03-04 19:35 PST, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2014-03-03 20:11:45 PST
Created attachment 225731 [details]
reproduction test case

Steps to reproduce:
1. Untar the attached test case.  You should get a debugger-statement.html and debugger-statement.js.  Load debugger-statement.html in the browser.
2. Open the WebInspector.
3. Set breakpoint in debugger-statement.js at lines 2 and 4.
4. Reload the page in the browser.

After the reload, the breakpoints are not re-enabled.

I put some prints in JSC::Debugger::setBreakpoint() and JSC::Debugger::clearBreakpoint(), and confirmed that the breakpoints were set, and then cleared when I did the reload.  However, after the reload, setBreakpoint() was not called to restore the breakpoints.  This means the issue happens above JSC.
Comment 1 Radar WebKit Bug Importer 2014-03-03 20:12:16 PST
<rdar://problem/16219327>
Comment 2 BJ Burg 2014-03-04 16:29:11 PST
Created attachment 225829 [details]
the patch
Comment 3 Mark Lam 2014-03-04 16:33:00 PST
Comment on attachment 225829 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225829&action=review

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:723
> +    scriptDebugServer().continueProgram();
> +    clearDebuggerBreakpointState();

Shouldn't you clear the breakpoints before continuing the program?
Comment 4 BJ Burg 2014-03-04 16:51:37 PST
(In reply to comment #3)
> (From update of attachment 225829 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225829&action=review
> 
> > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:723
> > +    scriptDebugServer().continueProgram();
> > +    clearDebuggerBreakpointState();
> 
> Shouldn't you clear the breakpoints before continuing the program?

That call is redundant with the clear/continue inside clearDebuggerBreakpointState(), and can be removed.
Comment 5 Joseph Pecoraro 2014-03-04 16:58:16 PST
Comment on attachment 225829 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225829&action=review

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:713
> +    ErrorString dummyError;
>      setOverlayMessage(&dummyError, nullptr);

setOverlayMessage currently does nothing. And I wouldn't associate this with "clearDebuggerBreakpointState". Maybe we should just remove it. Otherwise, leave it here and we will clean it up later.

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:723
>> +    clearDebuggerBreakpointState();
> 
> Shouldn't you clear the breakpoints before continuing the program?

I agree with Mark's comment. In fact, clearDebuggerBreakpointState already does a continue program, so you can remove the line from here. r=me with that change.
Comment 6 BJ Burg 2014-03-04 19:35:08 PST
Created attachment 225845 [details]
the patch (revised)
Comment 7 WebKit Commit Bot 2014-03-04 20:38:35 PST
Comment on attachment 225845 [details]
the patch (revised)

Clearing flags on attachment: 225845

Committed r165093: <http://trac.webkit.org/changeset/165093>
Comment 8 WebKit Commit Bot 2014-03-04 20:38:37 PST
All reviewed patches have been landed.  Closing bug.