Summary: | Web Inspector: Wrong function name next to scope | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||||||||||||||||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | bburg, buildbot, commit-queue, graouts, joepeck, mattbaker, nvasilyev, rniwa, ryanhaddad, timothy, webkit-bug-importer | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=159304 https://bugs.webkit.org/show_bug.cgi?id=159303 |
||||||||||||||||||||||||||||
Bug Depends on: | 159305 | ||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||
Attachments: |
|
Joe worked on this earlier in the release. It might be from those changes. Yeah, I've seen asserts from code in here that could lead to this. Okay, so this code is just plain wrong for non-nested functions. (1) Non-nested: - Call Frame 1 (bar) has 4 Scopes [ bar lexical, bar var, global lexical, global ] - Call Frame 2 (foo) has 4 Scopes [ foo lexical, foo var, global lexical, global ] > function foo() { > let x = 10; > bar(); > } > function bar() { > let z = 40; > debugger; > return 20; > } > foo(); (2) Nested: - Call Frame 1 (bar) has 6 Scopes [ bar lexical, bar var, foo lexical, foo var, global lexical, global ] - Call Frame 2 (foo) has 4 Scopes [ foo lexical, foo var, global lexical, global ] > function foo() { > let x = 10; > bar(); > function bar() { > let z = 40; > debugger; > return 20; > } > } > foo(); The algorithm in ScopeChainDetailsSidebarPanel.prototype._generateCallFramesSection was always assuming that the scope chain in a call frame always included the scope chain in an earlier call frame. That is _not_ the case when functions are not nested (1). In order to get this to work, we will have to include more data on the Scope objects themselves. Currently it only includes the `object` and `type`. It can include other details such as whether or not it is lexical, and ideally some `name`. That would be enough information for us to provide the correct sidebar details for scope chains in (1) and (2). Created attachment 281959 [details]
[PATCH] WIP
This approach adds a "name" to the Scope which the frontend uses to group scopes within a function. This passes the more involved tests.
This adds rare data to SymbolTable pointing to a CodeBlock during debugging. I don't know for certain if this pointer can go stale or not. I'll have to discuss this a bit with Saam. But I plan on cleaning this up a bit.
Comment on attachment 281959 [details] [PATCH] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=281959&action=review > Source/JavaScriptCore/debugger/DebuggerScope.cpp:224 > +String DebuggerScope::name() const > +{ > + SymbolTable* symbolTable = m_scope->symbolTable(); > + if (!symbolTable) > + return String(); > + > + CodeBlock* codeBlock = symbolTable->rareDataCodeBlock(); > + if (!codeBlock) > + return String(); > + > + return String::fromUTF8(codeBlock->inferredName()); > +} You may want more than a name to identify a function. Maybe name + line + column + sourceID? Created attachment 282017 [details]
[PATCH] Proposed Fix
I took Saam's advice and also included the location where possible. This gives us the ability to distinguish between identically named / unnamed functions, and we can provide a goto arrow in the UI if needed (not implemented yet).
Extended the test to test identically named functions.
Attachment 282017 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/debugger/DebuggerScope.h:30: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 282017 [details] [PATCH] Proposed Fix Attachment 282017 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1564573 New failing tests: inspector/debugger/paused-scopes.html Created attachment 282023 [details]
Archive of layout-test-results from ews101 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 282017 [details] [PATCH] Proposed Fix Attachment 282017 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1564595 New failing tests: inspector/debugger/paused-scopes.html Created attachment 282024 [details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 282028 [details]
[PATCH] Proposed Fix
Comment on attachment 282028 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=282028&action=review JSC bits look good to me. Just a few comments > Source/JavaScriptCore/debugger/DebuggerLocation.h:35 > +struct DebuggerLocation { Can this be any location? Or does it refer specifically to a function? If it specifically refers to a functions location, I think the name of the class should be modified to indicate that. If it can refer to any location, I think the fields should be renamed from "firstLine"=>"line" and "startColumn"=>"column". > Source/JavaScriptCore/runtime/SymbolTable.h:705 > + CodeBlock* m_codeBlock { nullptr }; Even though this is currently only set right after a new symbol table is allocated, I think it's better for this to be a WriteBarrier to future proof the setRareDataCodeBlock above. Created attachment 282260 [details]
[PATCH] Proposed Fix
Addressed Saam's comments.
Comment on attachment 282260 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=282260&action=review > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2483 > + clone->setRareDataCodeBlock(const_cast<CodeBlock*>(this)); Nit: I don't think you need this const_cast > Nit: I don't think you need this const_cast
Correct! New patch up.
Created attachment 282263 [details]
[PATCH] Proposed Fix
Comment on attachment 282263 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=282263&action=review > LayoutTests/inspector/debugger/paused-scopes.html:13 > + InspectorBackend.dumpInspectorProtocolMessages = true; Pls remove > LayoutTests/inspector/debugger/paused-scopes.html:48 > + return Promise.all(promises).then(() => { nice! Is there any reason to worry about nondeterminism here? I assume that the promises are going to be serviced in the same order every time... > LayoutTests/inspector/debugger/paused-scopes.html:55 > + let chain = Promise.resolve(null); I don't think the null argument is necessary. > LayoutTests/inspector/debugger/paused-scopes.html:58 > + return dumpCallFrame(callFrame); I would just inline this expression and drop the return and braces. > LayoutTests/inspector/debugger/paused-scopes.html:70 > + dumpCallFrames().then(() => { I think you can do dumpCallFrames().then(resolve, reject). We definitely need to call reject if dumping call frames failed for some reason. > LayoutTests/inspector/debugger/paused-scopes.html:85 > + resolve(); Same comments as above. Comment on attachment 282263 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=282263&action=review > Source/WebInspectorUI/UserInterface/Models/ScopeChainNode.js:62 > + makeLocalScope() Naming nit: convertToLocalScope(). makeXXX sounds like it creates a new object. > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:169 > + let scopes = callFrame.scopeChain.slice(); This is neat but might we want to do it outside of the sidebar view class? That would let us test the merging algorithm (and maybe use it again in other places if needed) > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:221 > + markAsBaseIfNeeded(scope, scopes[i+1]); scopes[i+1] will always be undefined in the first iteration of the loop. Is this intentional? > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:230 > + scopes.splice(i, 1); // Remove one of them. It feels yucky to iterate over scopes while mutating it. It's a programming error in C++ but merely dodgy in JS. Can we put the merged scope list into its own array and adjust the value of i to skip ahead? Since we don't seem to merge more than consecutive two scopes (maybe I'm wrong), I don't think we need to use an worklist/accumulator-like setup here. > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:245 > + scope.makeLocalScope(); ...so then this code would be scope.convertToLocalScope() > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:250 > + for (let scope of scopes) { Everything prior to here could be easily moved out of view code, which just needs to examine the merged scopes. > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:308 > + // FIXME: This just puts two ObjectTreeViews next to eachother, but that means Typo: eachother Created attachment 282382 [details]
[PATCH] Proposed Fix
Addressed Brian's feedback
Created attachment 282386 [details]
[PATCH] Proposed Fix
Tweaked test structure to be cleaner.
Comment on attachment 282386 [details]
[PATCH] Proposed Fix
r=me, Great work.
Re-opened since this is blocked by bug 159305 *** Bug 159304 has been marked as a duplicate of this bug. *** *** Bug 159303 has been marked as a duplicate of this bug. *** Links from duped bugs: LayoutTest inspector/model/scope-chain-node.html crashes https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/builds/6239 LayoutTest inspector/debugger/paused-scopes.html timing out https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r202680%20(6239)/results.html Created attachment 282478 [details]
[PATCH] Follow Up Changes
This got rolled out because of test failures / crashes with:
LayoutTests/inspector/debugger/paused-scopes.html
LayoutTests/inspector/model/scope-chain-node.html
I'm making these two changes:
1. JSSymbolTableObject was not declaring its own class info so it was getting JSScope's
and JSWithScope was unexpectedly a JSSymbolTableObject instead of a JSScope object.
2. Running the tests on debug builds causes timing differences that cause frontend
event differences (DebuggerManager, Paused, Resumed, CallFramesDidChange). Made the
tests more resilient, but a little slower to wait for explicit resumes.
This contains just the diffs I'm making to the original patch.
Comment on attachment 282478 [details] [PATCH] Follow Up Changes View in context: https://bugs.webkit.org/attachment.cgi?id=282478&action=review > Source/JavaScriptCore/runtime/JSSymbolTableObject.h:51 > + DECLARE_EXPORT_INFO; Probably doesn't need to be exported, this can probably be DECLARE_INFO. I'll try with that. (In reply to comment #30) > Comment on attachment 282478 [details] > [PATCH] Follow Up Changes > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282478&action=review > > > Source/JavaScriptCore/runtime/JSSymbolTableObject.h:51 > > + DECLARE_EXPORT_INFO; > > Probably doesn't need to be exported, this can probably be DECLARE_INFO. > I'll try with that. It does need to be, since JSGlobalObject eventually inherits from it. Created attachment 282484 [details]
[PATCH] Proposed Fix
Comment on attachment 282484 [details] [PATCH] Proposed Fix Clearing flags on attachment: 282484 Committed r202717: <http://trac.webkit.org/changeset/202717> All reviewed patches have been landed. Closing bug. *** Bug 19048 has been marked as a duplicate of this bug. *** |
Created attachment 280102 [details] image of bug I saw this repro on the STP before ShadowChicken. It also repros with ShadowChicken. I don't think the bug is ShadowChicken related. run this file: ``` function foo() { let x = 10; bar(); return baz(); } function bar() { let z = 40; debugger; return 20; } foo(); ``` And you will stop at the 'debugger' inside bar. Show the scope chain for 'bar' to the right. You will see the view for 'bar's scope chain looks as follows: `Closure Variables (foo)` `z : 40`