Bug 14190 - Breakpoints should persist across launches
Summary: Breakpoints should persist across launches
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Podivilov
URL:
Keywords:
Depends on: 40845
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-16 14:16 PDT by Justin Garcia
Modified: 2010-06-18 10:55 PDT (History)
8 users (show)

See Also:


Attachments
Proposed patch. (36.02 KB, patch)
2010-05-20 02:50 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Proposed patch. (29.40 KB, patch)
2010-05-20 03:07 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Breakpoints adding/removing logic refactored. (26.60 KB, patch)
2010-05-28 05:30 PDT, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
Updated after CR. (23.21 KB, patch)
2010-05-31 11:25 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Updated after CR. (26.40 KB, patch)
2010-05-31 11:29 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Proposed patch. (20.84 KB, patch)
2010-06-09 09:02 PDT, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
Proposed patch. (24.02 KB, patch)
2010-06-16 04:05 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Proposed patch. (24.03 KB, patch)
2010-06-16 04:40 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Rebase and minor fixes. (24.03 KB, patch)
2010-06-17 09:46 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Rebase and minor fixes. (23.45 KB, patch)
2010-06-17 09:49 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Proposed patch. (23.40 KB, patch)
2010-06-18 09:42 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 2007-06-16 14:16:25 PDT
It would be nice if breakpoints stuck around after restarting Drosera.
Comment 1 Timothy Hatcher 2008-05-17 09:24:13 PDT
Still applies to the new Web Inspector debugger. Moving to that component.
Comment 2 Pavel Podivilov 2010-05-19 01:56:32 PDT
I'll take it.
Comment 3 Pavel Podivilov 2010-05-20 02:50:00 PDT
Created attachment 56577 [details]
Proposed patch.
Comment 4 Pavel Podivilov 2010-05-20 03:07:41 PDT
Created attachment 56578 [details]
Proposed patch.
Comment 5 Yury Semikhatsky 2010-05-20 05:43:37 PDT
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
Comment 6 Pavel Feldman 2010-05-20 09:24:44 PDT
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.
Comment 7 Yury Semikhatsky 2010-05-21 02:00:50 PDT
(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?
Comment 8 Pavel Feldman 2010-05-25 15:31:15 PDT
> 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!
Comment 9 Yury Semikhatsky 2010-05-26 06:08:25 PDT
(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.
Comment 10 Pavel Feldman 2010-05-26 07:45:25 PDT
> 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.
Comment 11 Pavel Podivilov 2010-05-28 05:30:27 PDT
Created attachment 57318 [details]
Breakpoints adding/removing logic refactored. 

This is rebased version of original patch with saving to frontend settings stripped.
Comment 12 Pavel Feldman 2010-05-31 09:10:04 PDT
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).
Comment 13 Pavel Podivilov 2010-05-31 11:25:20 PDT
Created attachment 57483 [details]
Updated after CR.
Comment 14 Pavel Podivilov 2010-05-31 11:29:24 PDT
Created attachment 57484 [details]
Updated after CR.
Comment 15 Pavel Feldman 2010-05-31 12:18:58 PDT
Comment on attachment 57484 [details]
Updated after CR.

I'll land this.
Comment 16 Pavel Feldman 2010-05-31 12:47:05 PDT
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
Comment 17 Pavel Feldman 2010-05-31 12:48:12 PDT
Not closing for now: Preliminary refactoring landed, bug to be fixed with the subsequent patch shortly.
Comment 18 Eric Seidel (no email) 2010-06-02 02:27:32 PDT
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.
Comment 19 Pavel Podivilov 2010-06-09 09:02:28 PDT
Created attachment 58250 [details]
Proposed patch.
Comment 20 WebKit Review Bot 2010-06-09 09:37:38 PDT
Attachment 58250 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3236072
Comment 21 Pavel Feldman 2010-06-10 09:32:41 PDT
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.
Comment 22 Pavel Podivilov 2010-06-16 04:05:54 PDT
Created attachment 58870 [details]
Proposed patch.
Comment 23 Pavel Podivilov 2010-06-16 04:40:25 PDT
Created attachment 58878 [details]
Proposed patch.
Comment 24 WebKit Review Bot 2010-06-16 06:12:14 PDT
Attachment 58878 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3339196
Comment 25 Pavel Feldman 2010-06-16 08:17:00 PDT
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.
Comment 26 Yury Semikhatsky 2010-06-16 09:11:52 PDT
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.
Comment 27 Pavel Podivilov 2010-06-17 09:46:56 PDT
Created attachment 59004 [details]
Rebase and minor fixes.
Comment 28 Pavel Podivilov 2010-06-17 09:49:22 PDT
Created attachment 59005 [details]
Rebase and minor fixes.
Comment 29 WebKit Review Bot 2010-06-17 11:54:53 PDT
Attachment 59005 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3321322
Comment 30 Eric Seidel (no email) 2010-06-17 15:31:15 PDT
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.
Comment 31 Pavel Podivilov 2010-06-18 09:42:39 PDT
Created attachment 59126 [details]
Proposed patch.
Comment 32 Yury Semikhatsky 2010-06-18 09:52:16 PDT
Comment on attachment 59126 [details]
Proposed patch.

Clearing flags on attachment: 59126

Committed r61414: <http://trac.webkit.org/changeset/61414>
Comment 33 Yury Semikhatsky 2010-06-18 09:52:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 WebKit Review Bot 2010-06-18 10:21:19 PDT
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
Comment 35 Eric Seidel (no email) 2010-06-18 10:28:37 PDT
Looks like this was the culprit.   Broke Qt release.
Comment 36 Eric Seidel (no email) 2010-06-18 10:54:50 PDT
Looks like it was rolled out.
Comment 37 Eric Seidel (no email) 2010-06-18 10:55:56 PDT
Sorry, I misread, the build was fixed by https://trac.webkit.org/changeset/61419.