Bug 14190 – Breakpoints should persist across launches

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

[Justin Garcia]

It would be nice if breakpoints stuck around after restarting Drosera.

[Timothy Hatcher]

Still applies to the new Web Inspector debugger. Moving to that component.

[Pavel Podivilov]

I'll take it.

[Pavel Podivilov]

Created attachment 56577 [details]
Proposed patch.

[Pavel Podivilov]

Created attachment 56578 [details]
Proposed patch.

[Yury Semikhatsky]

Comment on attachment 56578 [details]
Proposed patch.

WebCore/inspector/front-end/BreakpointManager.js:105
 +          for (var line in breakpoints[breakpoint.url]) {
We may want to introduce an utility function for this

[Pavel Feldman]

Comment on attachment 56578 [details]
Proposed patch.

Since this change touches parts of the frontend that diverge in chromium, I'd rather land it manually after extensive testing.

[Yury Semikhatsky]

(In reply to comment #6)
> (From update of attachment 56578 [details])
> Since this change touches parts of the frontend that diverge in chromium, I'd rather land it manually after extensive testing.

Pavel discovered some serious issue with this approach. If you set some top-level breakpoint(somewhere in script code which is executed before onload event) on a page, close browser, reopen browser, open Web Inspector and navigate to the page the breakpoints won't be hit. This happens because breakpoints are restored asynchronously by BreakpointManager in response to parsedScriptSource event and by the time the breakpoints are restored the script has already executed. 

To fix this we need to restore breakpoints synchronously when script is parsed to make sure they are set before script started to execute. This means that it should be done on the backend side and we need to parse settings there.

I'm hesitant about landing this feature while the problem is not fixed. Guys, what do you think?

[Pavel Feldman]

> To fix this we need to restore breakpoints synchronously when script is parsed to make sure they are set before script started to execute. This means that it should be done on the backend side and we need to parse settings there.
> 

Yeah, I was actually mentioning it to the other Pavel earlier and I didn't really think it was such a big deal. Here is how I see it:

Scenario 1: Restarting the browser
1.1 User starts browser
1.2 User opens inspector
1.3 He navigates to a page in a non-blocking manner
1.4 In the inspector, chrome-alike notification bar appears suggesting that there are breakpoints in the store that match the site.
1.5 User either accepts or declines breakpoints restore and reloads if that is necessary
1.6 From that moment, our restore-breakpoints-on-reload backend code handles the rest.

Scenario 2: Reopening the inspector
2.1 User re-opens inspector
2.2 We detect that there are matching breakpoints in the store
2.3 We silently restore all of them
2.4 They will hit from now on or on reload

Just make sure both scenarios work and we are good! I do realize that 1.4 might require some UI work, but we should not silently restore breakpoints upon browser restart. One other thing, please file a bug on support for shifts in line numbers (i.e. we should map breakpoints not to source file + line number, but to the content as well so that fuzz=3 worked!

[Yury Semikhatsky]

(In reply to comment #8)
> > To fix this we need to restore breakpoints synchronously when script is parsed to make sure they are set before script started to execute. This means that it should be done on the backend side and we need to parse settings there.
> > 
> 
> Yeah, I was actually mentioning it to the other Pavel earlier and I didn't really think it was such a big deal. Here is how I see it:
> 
> Scenario 1: Restarting the browser
> 1.1 User starts browser
> 1.2 User opens inspector
> 1.3 He navigates to a page in a non-blocking manner
> 1.4 In the inspector, chrome-alike notification bar appears suggesting that there are breakpoints in the store that match the site.
> 1.5 User either accepts or declines breakpoints restore and reloads if that is necessary
> 1.6 From that moment, our restore-breakpoints-on-reload backend code handles the rest.
> 
> Scenario 2: Reopening the inspector
> 2.1 User re-opens inspector
> 2.2 We detect that there are matching breakpoints in the store
> 2.3 We silently restore all of them
> 2.4 They will hit from now on or on reload
> 
There is one more interesting scenario to consider:

Scenario 3: Navigating to another URL
3.1 User starts browser
3.2 User opens inspector
3.3 He navigates to a page in a non-blocking manner
3.4 Breakpoints are restored(with user approval or silently)
3.5 User navigates to a page from a different domain which may be opened in a new render process. 
3.6 At this point we should restore all breakpoints before the page started loading.

3.6 implies that each script should wait until frontend completes restoring all breakpoints(if we have breakpoint restoring managed by the frontend).


> Just make sure both scenarios work and we are good! I do realize that 1.4 might require some UI work, but we should not silently restore breakpoints upon browser restart.

I really don't like the idea of the annoying notifications on the browser restart. I'd rather silently restore them and add a button "delete all breakpoints"(as  Pavel Podivilov suggested) so that the user has a chance to manually delete all the breakpoints in case he doesn't need them anymore. This would be less intrusive than showing notification bar on each browser restart after debugging session.


> One other thing, please file a bug on support for shifts in line numbers (i.e. we should map breakpoints not to source file + line number, but to the content as well so that fuzz=3 worked!
Do we really want this sort of behavior? It's much less obvious from the user stand point. We'd have to somehow let user know why a breakpoint can not be restored in a given script. The feature may not worth complication it'd introduce into the breakpoints code.

[Pavel Feldman]

> 3.5 User navigates to a page from a different domain which may be opened in a new render process. 
> 3.6 At this point we should restore all breakpoints before the page started loading.
> 
> 3.6 implies that each script should wait until frontend completes restoring all breakpoints(if we have breakpoint restoring managed by the frontend).
> 

At first I though I disagree. I'd say load page non blocking, restore breakpoints and let user reload should he need this. Rationale: we should not block since breakpoints restore might be not desired during the navigation. But then I thought that it might be really helpful in debuggin transitions. So I tend to agree with you now. So the solution would be to parse / understand stored breakpoints on the backend side and merge breakpoint restore code with present onload's restorebreakpoints.

> I really don't like the idea of the annoying notifications on the browser restart. I'd rather silently restore them and add a button "delete all breakpoints"(as  Pavel Podivilov suggested) so that the user has a chance to manually delete all the breakpoints in case he doesn't need them anymore. This would be less intrusive than showing notification bar on each browser restart after debugging session.

Not sure I am with you here. I tend to keep browser open while I debug some user scenario. Between the browser sessions, my debug sessions also change, so breakpoints should go away. It is like IDE that is not persisting breakpoints between starts. And I think notification is appropriate here.

> Do we really want this sort of behavior? It's much less obvious from the user stand point. We'd have to somehow let user know why a breakpoint can not be restored in a given script. The feature may not worth complication it'd introduce into the breakpoints code.

We definitely want that since otherwise any line shift will break all breakpoints. This does not happen in the IDE since IDE shifts breakpoints on edits. But in our case editing is separate from browser, hence it is likely to spoil all the breaks. I don't think that fuzz is hard to implement.

[Pavel Podivilov]

Created attachment 57318 [details]
Breakpoints adding/removing logic refactored. 

This is rebased version of original patch with saving to frontend settings stripped.

[Pavel Feldman]

Comment on attachment 57318 [details]
Breakpoints adding/removing logic refactored. 

A bunch of renames below. Otherwise looks very good.

WebCore/inspector/front-end/BreakpointManager.js:2
 +   * Copyright (C) 2008 Apple Inc. All Rights Reserved.
Please append Google copyright 2010 string.

WebCore/inspector/front-end/BreakpointManager.js:34
 +          return new WebInspector.Breakpoint(sourceID, sourceURL, line, enabled, condition, this);
Nit: 'this' is usually a first parameter in the list.

WebCore/inspector/front-end/BreakpointManager.js:40
 +             return;
Poor indentation

WebCore/inspector/front-end/BreakpointManager.js:55
 +      getBreakpointsBySourceID: function(sourceID) {
Place { on the next line.

WebCore/inspector/front-end/BreakpointManager.js:64
 +      getBreakpointsByURL: function(url) {
Place { on the next line.

WebCore/inspector/front-end/BreakpointManager.js:73
 +      reset: function() {
Place { on the next line.

WebCore/inspector/front-end/BreakpointManager.js:98
 +      this._manager = manager;
this._breakpointManager

WebCore/inspector/front-end/BreakpointManager.js:77
 +      _saveBreakpointToBackend: function(breakpoint)
_setBreakpointOnBackend

WebCore/inspector/front-end/BreakpointManager.js:82
 +      _removeBreakpointFromBackend: function(breakpoint)
_removeBreakpointFromBackend

WebCore/inspector/front-end/BreakpointManager.js:64
 +      getBreakpointsByURL: function(url) {
In WebCore, get is not used, so this one would be breakpointsForURL


WebCore/inspector/front-end/BreakpointManager.js:32
 +      createBreakpoint: function(sourceID, sourceURL, line, enabled, condition)
You always add breakpoints after creation. Consider merging these methods.

WebCore/inspector/front-end/ScriptsPanel.js:346
 +          var breakpoints = WebInspector.breakpointManager.getBreakpointsBySourceID(sourceID);
You only use getBreakpointsBySourceID here. Consider encapsulating the whole routine (removeBreakpointsWithSourceID).

[Pavel Podivilov]

Created attachment 57483 [details]
Updated after CR.

[Pavel Podivilov]

Created attachment 57484 [details]
Updated after CR.

[Pavel Feldman]

Comment on attachment 57484 [details]
Updated after CR.

I'll land this.

[Pavel Feldman]

Comment on attachment 57484 [details]
Updated after CR.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/inspector/front-end/Breakpoint.js
	A	WebCore/inspector/front-end/BreakpointManager.js
	M	WebCore/inspector/front-end/BreakpointsSidebarPane.js
	M	WebCore/inspector/front-end/Object.js
	M	WebCore/inspector/front-end/ScriptView.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	M	WebCore/inspector/front-end/SourceFrame.js
	M	WebCore/inspector/front-end/SourceView.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
Committed r60450

[Pavel Feldman]

Not closing for now: Preliminary refactoring landed, bug to be fixed with the subsequent patch shortly.

[Eric Seidel (no email)]

Comment on attachment 56578 [details]
Proposed patch.

Cleared Yury Semikhatsky's review+ from obsolete attachment 56578 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Pavel Podivilov]

Created attachment 58250 [details]
Proposed patch.

[WebKit Review Bot]

Attachment 58250 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3236072

[Pavel Feldman]

Comment on attachment 58250 [details]
Proposed patch.

Almost ready to land. Couple of nits below.

WebCore/inspector/InspectorController.cpp:1753
 +      String key = String(breakpointsInspectorSettingPrefix) + m_mainResource->requestURL();
Please use String::format

WebCore/inspector/InspectorController.cpp:1765
 +          sourceBreakpointsFromInspectorObject(breakpointsForURL, &sourceBreakpoints);
I would declare this as a static method on SourceBreakpoint. No need to add stuff to WebCore namespace.


WebCore/inspector/ScriptBreakpoint.cpp:2
 +   * Copyright (C) 2009 Apple Inc. All rights reserved.
Please use 2010 Google only, Apple has not yet contributed to this file.

WebCore/inspector/front-end/BreakpointManager.js:55
 +          this._saveBreakpointOnBackend(breakpoint);
Please make sure one time breakpoints work after your change and are not persisted.

[Pavel Podivilov]

Created attachment 58870 [details]
Proposed patch.

[Pavel Podivilov]

Created attachment 58878 [details]
Proposed patch.

[WebKit Review Bot]

Attachment 58878 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3339196

[Pavel Feldman]

Comment on attachment 58878 [details]
Proposed patch.

Looks sane to me. Given that we have no tests for this, please test it manually well. Also might be worth asking Yury to look at it.

[Yury Semikhatsky]

Comment on attachment 58878 [details]
Proposed patch.

WebCore/inspector/InspectorController.cpp:1724
 +      loadBreakpoints();
do you really need to load the breakpoints again here?

WebCore/inspector/InspectorController.cpp:1818
 +          SourceBreakpoints& sourceBreakpoints = m_stickyBreakpoints.set(it->first, SourceBreakpoints()).first->second;
Please split this line into more readable parts.

Looks good to me.

[Pavel Podivilov]

Created attachment 59004 [details]
Rebase and minor fixes.

[Pavel Podivilov]

Created attachment 59005 [details]
Rebase and minor fixes.

[WebKit Review Bot]

Attachment 59005 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3321322

[Eric Seidel (no email)]

Comment on attachment 58878 [details]
Proposed patch.

Cleared Pavel Feldman's review+ from obsolete attachment 58878 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Pavel Podivilov]

Created attachment 59126 [details]
Proposed patch.

[Yury Semikhatsky]

Comment on attachment 59126 [details]
Proposed patch.

Clearing flags on attachment: 59126

Committed r61414: <http://trac.webkit.org/changeset/61414>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61414 might have broken Qt Linux Release minimal
The following changes are on the blame list:
http://trac.webkit.org/changeset/61416
http://trac.webkit.org/changeset/61414
http://trac.webkit.org/changeset/61415

[Eric Seidel (no email)]

Looks like this was the culprit.   Broke Qt release.

[Eric Seidel (no email)]

Looks like it was rolled out.

[Eric Seidel (no email)]

Sorry, I misread, the build was fixed by https://trac.webkit.org/changeset/61419.