Bug 129655

Summary: Inspector does not restore breakpoints after a page reload
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: Web InspectorAssignee: BJ Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, joepeck, mark.lam, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
reproduction test case
none
the patch
none
the patch (revised) none

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.