| Summary: | Web Inspector: shown() called on a content view when stepping over an instruction in the debugger | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||
| Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | burg, commit-queue, graouts, joepeck, timothy, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Saam Barati
2014-07-25 16:19:57 PDT
It wasn't intended to be called multiple times. It seems like the main problem is that when stepping over an instruction, the debugger on the JSC side skips to the next instruction, and then emits the paused event. When the Inspector sees this pause event, it adds a new backForwardEntry and shows that new backForwardEntry, even if the content view it shows is for the same Resource. So, if stepping over every instruction in a JS file, there will be backForwardEntries for every instruction that's been stepped over. So clicking the back button at the top of the content browser will just show older content views that are really just the same file that we've been stepping over instructions within that file. The problem, then, doesn't seem to be that shown() is called multiple times on the same content view (because new content views for the same resource are repeatedly created), but rather, that we create new content views for every instruction we step over. This also seems bad from the user experience point of view of stepping over ten instructions, and then wanting to click back to go back to the previous JS file, and having to click back 10 times just to skip over these 10 content views that were created for each pause event. I'm not sure what the correct solution for this is, but I think the way it is set up now isn't the correct architecture. What do you all think? The back/forward list is not just resources (views) but also scroll positions and other cookie data. So the back button should be jumping around in the view to previous scroll positions. This is how Xcode behaves. shown() should still probably not be called if the view does not change. We probably should not push new back forward entries for each pause/resume if the scroll position didn't change. (In reply to comment #5) > We probably should not push new back forward entries for each pause/resume if the scroll position didn't change. Yeah, that seems reasonable to me. I think saving them for each pause/resume is overkill when stepping over instructions. It looks like this code is at fault:
if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie))
return currentEntry.contentView;
It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions.
(In reply to comment #3) > The problem, then, doesn't seem to be that shown() is called multiple times on the same content view (because new content views for the same resource are repeatedly created), but rather, that we create new content views for every instruction we step over. Do we really create a new ContentView? > This also seems bad from the user experience point of view of stepping over ten instructions, and then wanting to click back to go back to the previous JS file, and having to click back 10 times just to skip over these 10 content views that were created for each pause event. That sounds unintentional. I think the back forward list should be for specific user events. 1. Showing a ContentView, by clicking a Script/Resource Tree Element in the source sidebar 2. Jumping to a location by clicking a Breakpoint Tree Element in the debugger sidebar 3. Jumping to a location by clicking a CallFrame Tree Element in the debugger sidebar 4. Jumping to a location by clicking a (source:line:column) link anywhere in the UI 5. Hiding a ContentView, this may want to add a new entry or replace the current entry I think stepping in the debugger it would be unexpected that that adds a new back/forward entry other than the possibility of normal navigation with (1) or (5). (In reply to comment #8) > the source sidebar Resource* sidebar (In reply to comment #7) > It looks like this code is at fault: > > if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie)) > return currentEntry.contentView; > > > It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions. Okay, I'll look into it. I was actually looking at that code path earlier, and it actually does what it's supposed two thirds of the time. It seems like that code path is travelled down 3 times for each step-over, and on one of the three paths when stepping over, that test fails. (In reply to comment #10) > (In reply to comment #7) > > It looks like this code is at fault: > > > > if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie)) > > return currentEntry.contentView; > > > > > > It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions. > > Okay, I'll look into it. I was actually looking at that code path earlier, and it actually does what it's supposed two thirds of the time. It seems like that code path is travelled down 3 times for each step-over, and on one of the three paths when stepping over, that test fails. Three times sounds like two too many. (In reply to comment #8) > > Do we really create a new ContentView? > I'm not 100% sure on this. I think the more accurate statement is new BackForwardEntries are created. > > That sounds unintentional. I think the back forward list should be for specific user events. > > 1. Showing a ContentView, by clicking a Script/Resource Tree Element in the source sidebar > 2. Jumping to a location by clicking a Breakpoint Tree Element in the debugger sidebar > 3. Jumping to a location by clicking a CallFrame Tree Element in the debugger sidebar > 4. Jumping to a location by clicking a (source:line:column) link anywhere in the UI > 5. Hiding a ContentView, this may want to add a new entry or replace the current entry > > I think stepping in the debugger it would be unexpected that that adds a new back/forward entry other than the possibility of normal navigation with (1) or (5). Yeah that seems reasonable. I would also add that a new BackForwardEntry should be added when a debugger paused event causes you to move to a completely new location within the same file. (In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #7) > > > It looks like this code is at fault: > > > > > > if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie)) > > > return currentEntry.contentView; > > > > > > > > > It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions. > > > > Okay, I'll look into it. I was actually looking at that code path earlier, and it actually does what it's supposed two thirds of the time. It seems like that code path is travelled down 3 times for each step-over, and on one of the three paths when stepping over, that test fails. > > Three times sounds like two too many. Yeah this is a stack trace after stepping over an instruction: file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22: CONSOLE TRACE 0: showContentView(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22) 1: showContentViewForRepresentedObject(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentBrowser.js:158:58) 2: showSourceCode(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:181:72) 3: showSourceCodeLocation(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:186:28) 4: _treeElementSelected(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:525:69) 5: (unknown)([native code]) 6: select(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/TreeOutline.js:976:34) 7: _debuggerCallFramesDidChange(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:395:39) 8: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:180:55) 9: dispatchEventToListeners(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:187:17) 10: debuggerDidPause(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Controllers/DebuggerManager.js:379:38) 11: paused(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/DebuggerObserver.js:58:54) 12: dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:309:42) 13: _dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:215:32) 14: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:89:32) 15: dispatchNextQueuedMessageFromBackend(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/MessageDispatcher.js:31:34) 16: (unknown)([native code]) 17: global code(file:///Volumes/Data/test.js:2:1) file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22: CONSOLE TRACE 0: showContentView(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22) 1: _showContentViewForIdentifier(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceClusterContentView.js:236:57) 2: restoreFromCookie(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceClusterContentView.js:147:61) 3: _restoreFromCookie(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Models/BackForwardEntry.js:77:43) 4: prepareToShow(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Models/BackForwardEntry.js:57:32) 5: _showEntry(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:402:28) 6: showBackForwardEntryForIndex(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:203:24) 7: showContentView(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:180:42) 8: showContentViewForRepresentedObject(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentBrowser.js:158:58) 9: showSourceCode(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:181:72) 10: showSourceCodeLocation(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:186:28) 11: _treeElementSelected(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:525:69) 12: (unknown)([native code]) 13: select(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/TreeOutline.js:976:34) 14: _debuggerCallFramesDidChange(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:395:39) 15: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:180:55) 16: dispatchEventToListeners(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:187:17) 17: debuggerDidPause(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Controllers/DebuggerManager.js:379:38) 18: paused(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/DebuggerObserver.js:58:54) 19: dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:309:42) 20: _dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:215:32) 21: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:89:32) 22: dispatchNextQueuedMessageFromBackend(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/MessageDispatcher.js:31:34) 23: (unknown)([native code]) 24: global code(file:///Volumes/Data/test.js:2:1) file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22: CONSOLE TRACE 0: showContentView(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22) 1: _showContentViewForIdentifier(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceClusterContentView.js:236:57) 2: shown(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceClusterContentView.js:130:43) 3: prepareToShow(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Models/BackForwardEntry.js:60:31) 4: _showEntry(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:402:28) 5: showBackForwardEntryForIndex(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:203:24) 6: showContentView(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:180:42) 7: showContentViewForRepresentedObject(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentBrowser.js:158:58) 8: showSourceCode(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:181:72) 9: showSourceCodeLocation(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:186:28) 10: _treeElementSelected(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:525:69) 11: (unknown)([native code]) 12: select(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/TreeOutline.js:976:34) 13: _debuggerCallFramesDidChange(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:395:39) 14: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:180:55) 15: dispatchEventToListeners(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:187:17) 16: debuggerDidPause(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Controllers/DebuggerManager.js:379:38) 17: paused(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/DebuggerObserver.js:58:54) 18: dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:309:42) 19: _dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:215:32) 20: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:89:32) 21: dispatchNextQueuedMessageFromBackend(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/MessageDispatcher.js:31:34) 22: (unknown)([native code]) 23: global code(file:///Volumes/Data/test.js:2:1) (In reply to comment #13) > (In reply to comment #11) > > (In reply to comment #10) > > > (In reply to comment #7) > > > > It looks like this code is at fault: > > > > > > > > if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie)) > > > > return currentEntry.contentView; > > > > > > > > > > > > It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions. > > > > > > Okay, I'll look into it. I was actually looking at that code path earlier, and it actually does what it's supposed two thirds of the time. It seems like that code path is travelled down 3 times for each step-over, and on one of the three paths when stepping over, that test fails. > > > > Three times sounds like two too many. > > Yeah this is a stack trace after stepping over an instruction: > > [snip] So this is a case of ResourceClusterContentView showing a sub-view, so this is likely correct. I think we should add an isEqual function to BackForwardEntry that also looks at _contentView, _cookie and _scrollPositions. (In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #11) > > > (In reply to comment #10) > > > > (In reply to comment #7) > > > > > It looks like this code is at fault: > > > > > > > > > > if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie)) > > > > > return currentEntry.contentView; > > > > > > > > > > > > > > > It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions. > > > > > > > > Okay, I'll look into it. I was actually looking at that code path earlier, and it actually does what it's supposed two thirds of the time. It seems like that code path is travelled down 3 times for each step-over, and on one of the three paths when stepping over, that test fails. > > > > > > Three times sounds like two too many. > > > > Yeah this is a stack trace after stepping over an instruction: > > > > [snip] > > So this is a case of ResourceClusterContentView showing a sub-view, so this is likely correct. > > I think we should add an isEqual function to BackForwardEntry that also looks at _contentView, _cookie and _scrollPositions. While that would fix some other cases, it will not fix views that use CodeMirror since _scrollPositions is not used for those. So there is another bug with CodeMirror views not saving scroll positions. CodeMirror views do save the last "jump" location to the cookie, which will be used for this with a showSourceCodeLocation call. And the cookie is already compared. So i think the behavior is correct as-is, where each jump to a new line in the same view pushes a new b/f entry. Not really the best behavior. Still shown() should not be called if it is already showing the view. Created attachment 235724 [details]
Idea for patch
What do you guys think of this patch?
It solves the issues I'm having with shown() being called multiple times. But maybe calling _restoreFromCookie() is an abstraction violation. But this is the essence of what a patch to fix this would be, I think.
Comment on attachment 235724 [details] Idea for patch View in context: https://bugs.webkit.org/attachment.cgi?id=235724&action=review Looks right! > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:205 > + currentEntry._restoreFromCookie(); Yeah, restoreFromCookie should drop the underscore and be a public method if this needs to call it. Comment on attachment 235724 [details]
Idea for patch
Actually it might be best to make a helper to do this. There are a couple other places where we do the _hideEntry(previousEntry) / _showEntry(currentEntry) dance.
(In reply to comment #17) > (From update of attachment 235724 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=235724&action=review > > Looks right! > > > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:205 > > + currentEntry._restoreFromCookie(); > > Yeah, restoreFromCookie should drop the underscore and be a public method if this needs to call it. Okay. I'll probably end up doing that. Is it the intention of restoreFromCookie to be called more than once? Or maybe should there be another interface to updating the lineNumber info. (In reply to comment #18) > (From update of attachment 235724 [details]) > Actually it might be best to make a helper to do this. There are a couple other places where we do the _hideEntry(previousEntry) / _showEntry(currentEntry) dance. Okay. I'll make this a helper. (In reply to comment #20) > (In reply to comment #18) > > (From update of attachment 235724 [details] [details]) > > Actually it might be best to make a helper to do this. There are a couple other places where we do the _hideEntry(previousEntry) / _showEntry(currentEntry) dance. > > Okay. I'll make this a helper. It actually doesn't seem like there is a good way to generalize this to all other place that call both _showEntry/_hideEntry Created attachment 235750 [details]
another patch proposition
This could also be a way to implement that patch, that passes in a flag indicating whether BackForwardEntry should call shown().
Which do you all like better?
Comment on attachment 235750 [details]
another patch proposition
I like this.
Comment on attachment 235750 [details] another patch proposition View in context: https://bugs.webkit.org/attachment.cgi?id=235750&action=review > Source/WebInspectorUI/UserInterface/Models/BackForwardEntry.js:57 > this._restoreFromCookie(); Did we also determine that _restoreFromCookie might call shown()? That was being tracked separately though. (In reply to comment #24) > (From update of attachment 235750 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=235750&action=review > > > Source/WebInspectorUI/UserInterface/Models/BackForwardEntry.js:57 > > this._restoreFromCookie(); > > Did we also determine that _restoreFromCookie might call shown()? That was being tracked separately though. I haven't looked into it, specifically. Anecdotally, this hasn't happened in my testing of the above patches. That's not to say it might not. Created attachment 236000 [details] patch I've tested this patch, and it solves the problem of shown not being repeatedly called when stepping over instructions in the debugger, but it doesn't solve all the problems of shown being called multiple times. I think for further fixing the redundant calls to shown, I should do more investigation on: https://bugs.webkit.org/show_bug.cgi?id=135000. Comment on attachment 236000 [details] patch Clearing flags on attachment: 236000 Committed r172038: <http://trac.webkit.org/changeset/172038> All reviewed patches have been landed. Closing bug. |