WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
14190
Breakpoints should persist across launches
https://bugs.webkit.org/show_bug.cgi?id=14190
Summary
Breakpoints should persist across launches
Justin Garcia
Reported
2007-06-16 14:16:25 PDT
It would be nice if breakpoints stuck around after restarting Drosera.
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
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Timothy Hatcher
Comment 1
2008-05-17 09:24:13 PDT
Still applies to the new Web Inspector debugger. Moving to that component.
Pavel Podivilov
Comment 2
2010-05-19 01:56:32 PDT
I'll take it.
Pavel Podivilov
Comment 3
2010-05-20 02:50:00 PDT
Created
attachment 56577
[details]
Proposed patch.
Pavel Podivilov
Comment 4
2010-05-20 03:07:41 PDT
Created
attachment 56578
[details]
Proposed patch.
Yury Semikhatsky
Comment 5
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
Pavel Feldman
Comment 6
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.
Yury Semikhatsky
Comment 7
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?
Pavel Feldman
Comment 8
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!
Yury Semikhatsky
Comment 9
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.
Pavel Feldman
Comment 10
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.
Pavel Podivilov
Comment 11
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.
Pavel Feldman
Comment 12
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).
Pavel Podivilov
Comment 13
2010-05-31 11:25:20 PDT
Created
attachment 57483
[details]
Updated after CR.
Pavel Podivilov
Comment 14
2010-05-31 11:29:24 PDT
Created
attachment 57484
[details]
Updated after CR.
Pavel Feldman
Comment 15
2010-05-31 12:18:58 PDT
Comment on
attachment 57484
[details]
Updated after CR. I'll land this.
Pavel Feldman
Comment 16
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
Pavel Feldman
Comment 17
2010-05-31 12:48:12 PDT
Not closing for now: Preliminary refactoring landed, bug to be fixed with the subsequent patch shortly.
Eric Seidel (no email)
Comment 18
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
.
Pavel Podivilov
Comment 19
2010-06-09 09:02:28 PDT
Created
attachment 58250
[details]
Proposed patch.
WebKit Review Bot
Comment 20
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
Pavel Feldman
Comment 21
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.
Pavel Podivilov
Comment 22
2010-06-16 04:05:54 PDT
Created
attachment 58870
[details]
Proposed patch.
Pavel Podivilov
Comment 23
2010-06-16 04:40:25 PDT
Created
attachment 58878
[details]
Proposed patch.
WebKit Review Bot
Comment 24
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
Pavel Feldman
Comment 25
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.
Yury Semikhatsky
Comment 26
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.
Pavel Podivilov
Comment 27
2010-06-17 09:46:56 PDT
Created
attachment 59004
[details]
Rebase and minor fixes.
Pavel Podivilov
Comment 28
2010-06-17 09:49:22 PDT
Created
attachment 59005
[details]
Rebase and minor fixes.
WebKit Review Bot
Comment 29
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
Eric Seidel (no email)
Comment 30
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
.
Pavel Podivilov
Comment 31
2010-06-18 09:42:39 PDT
Created
attachment 59126
[details]
Proposed patch.
Yury Semikhatsky
Comment 32
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
>
Yury Semikhatsky
Comment 33
2010-06-18 09:52:30 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 34
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
Eric Seidel (no email)
Comment 35
2010-06-18 10:28:37 PDT
Looks like this was the culprit. Broke Qt release.
Eric Seidel (no email)
Comment 36
2010-06-18 10:54:50 PDT
Looks like it was rolled out.
Eric Seidel (no email)
Comment 37
2010-06-18 10:55:56 PDT
Sorry, I misread, the build was fixed by
https://trac.webkit.org/changeset/61419
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug