Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2415073003:
[debug] [reland] Consistently use script from FrameMirror (Closed)
|
Created:
4 years, 2 months ago by Clemens Hammacher Modified:
4 years, 2 months ago CC:
devtools-reviews_chromium.org, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[debug] [reland] Consistently use script from FrameMirror
... instead of getting it from the FunctionMirror. For WASM frames
(including asm.js -> WASM), the function is either unresolved or does
not contain the script.
The added test case failed before this CL.
R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org
BUG=v8:4203, chromium:656622
Committed: https://crrev.com/ce32e2ffd835062d764f3c0ee6a32543417cb615
Committed: https://crrev.com/ea51b8a764a80f047979e577c9d5945f0c83f362
Cr-Original-Commit-Position: refs/heads/master@{#40348}
Cr-Commit-Position: refs/heads/master@{#40387}
Patch Set 1 #
Total comments: 4
Patch Set 2 : add inspector test for asm.js stack traces #Patch Set 3 : address comments and minor other fixes #
Dependent Patchsets: Messages
Total messages: 41 (24 generated)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [debug] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either not set or does not contain the script. R=jgruber@chromium.org, yangguo@chromium.org BUG=v8:4203 ========== to ========== [debug] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. R=jgruber@chromium.org, yangguo@chromium.org BUG=v8:4203 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yangguo@chromium.org changed reviewers: + kozyatinskiy@chromium.org
+kozyatinskiy
Can we add inspector test for this?
Inspector tests are located in test/inspector.
Each test contains js file with test code and -expected.txt file with expected
results.
You need to create both of them and then you can run all tests with
"tools/run-tests.py inspector".
You can add script in inspected context with:
InspectorTest.addScript(`
function testFunction()
{
// call wasm module with debug break
}
`);
And then enable debugger, set paused listener, evaluate testFunction and
complete test:
Protocol.Debugger.enable();
Protocol.Debugger.onPaused(message => /* check stack trace from message and
resume execution, you can use InspectorTest.logMessage to dump entire message
*/);
Protocol.Runtime.evaluate({ expression: "testFunction()"
}).then(InspectorTest.completeTest);
At the end of the test you need to call InspectorTest.completeTest() to finish
it. You can call it in then callback or immediately after dumping stack in
onPaused callback.
https://codereview.chromium.org/2415073003/diff/1/src/inspector/debugger-scri...
File src/inspector/debugger-script.js (right):
https://codereview.chromium.org/2415073003/diff/1/src/inspector/debugger-scri...
src/inspector/debugger-script.js:496: var script = /** @type {!FunctionMirror}
*/(funcMirror).script();
Does WASM frame contain scope chain? If yes, then should we use
frameMirror.script() here too?
https://codereview.chromium.org/2415073003/diff/1/src/inspector/debugger-scri...
src/inspector/debugger-script.js:519: var script = frameMirror.script();
Please store scriptMirror at the beginning of this function and then use it.
Using frameMirror in lazyDetails function can be dangerous since it can be
called not on pause. I think it's a root of crash in layout tests on blink bot.
Description was changed from ========== [debug] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. R=jgruber@chromium.org, yangguo@chromium.org BUG=v8:4203 ========== to ========== [debug] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203 ==========
clemensh@chromium.org changed reviewers: + titzer@chromium.org - jgruber@chromium.org
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, that was really helpful information. I wanted to write a test case anyway, but was unsure how we usually test this interface. I added a test case which fails before this CL, but passes after. https://codereview.chromium.org/2415073003/diff/1/src/inspector/debugger-scri... File src/inspector/debugger-script.js (right): https://codereview.chromium.org/2415073003/diff/1/src/inspector/debugger-scri... src/inspector/debugger-script.js:496: var script = /** @type {!FunctionMirror} */(funcMirror).script(); On 2016/10/13 16:06:18, kozyatinskiy wrote: > Does WASM frame contain scope chain? If yes, then should we use > frameMirror.script() here too? No, there is no scope chain for WASM frames. Also, I think the point of this code is to have different functions in the scope chain, thus we also need to select the correct script per scope. https://codereview.chromium.org/2415073003/diff/1/src/inspector/debugger-scri... src/inspector/debugger-script.js:519: var script = frameMirror.script(); On 2016/10/13 16:06:18, kozyatinskiy wrote: > Please store scriptMirror at the beginning of this function and then use it. > Using frameMirror in lazyDetails function can be dangerous since it can be > called not on pause. I think it's a root of crash in layout tests on blink bot. Done. I also removed two checks for the existence of a script, since we now always have a Script available (I double-checked this in runtime-debug.cc).
thank you! lgtm.
The CQ bit was checked by clemensh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/26532)
@Yang PTAL
On 2016/10/17 07:15:48, Clemens Hammacher wrote: > @Yang PTAL lgtm.
The CQ bit was checked by clemensh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [debug] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203 ========== to ========== [debug] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [debug] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203 ========== to ========== [debug] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203 Committed: https://crrev.com/ce32e2ffd835062d764f3c0ee6a32543417cb615 Cr-Commit-Position: refs/heads/master@{#40348} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ce32e2ffd835062d764f3c0ee6a32543417cb615 Cr-Commit-Position: refs/heads/master@{#40348}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2427633002/ by machenbach@chromium.org. The reason for reverting is: https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui... https://github.com/v8/v8/wiki/Blink-layout-tests.
Message was sent while issue was closed.
Description was changed from ========== [debug] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203 Committed: https://crrev.com/ce32e2ffd835062d764f3c0ee6a32543417cb615 Cr-Commit-Position: refs/heads/master@{#40348} ========== to ========== [debug] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203, chromium:656622 Committed: https://crrev.com/ce32e2ffd835062d764f3c0ee6a32543417cb615 Cr-Commit-Position: refs/heads/master@{#40348} ==========
Message was sent while issue was closed.
Description was changed from ========== [debug] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203, chromium:656622 Committed: https://crrev.com/ce32e2ffd835062d764f3c0ee6a32543417cb615 Cr-Commit-Position: refs/heads/master@{#40348} ========== to ========== [debug] [reland] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203, chromium:656622 Committed: https://crrev.com/ce32e2ffd835062d764f3c0ee6a32543417cb615 Cr-Commit-Position: refs/heads/master@{#40348} ==========
On 2016/10/17 12:37:42, machenbach (slow) wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.chromium.org/2427633002/ by mailto:machenbach@chromium.org. > > The reason for reverting is: > https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui... > > https://github.com/v8/v8/wiki/Blink-layout-tests. The respective test is now marked NeedsManualRebaseline; relanding this CL.
The CQ bit was checked by clemensh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [debug] [reland] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203, chromium:656622 Committed: https://crrev.com/ce32e2ffd835062d764f3c0ee6a32543417cb615 Cr-Commit-Position: refs/heads/master@{#40348} ========== to ========== [debug] [reland] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203, chromium:656622 Committed: https://crrev.com/ce32e2ffd835062d764f3c0ee6a32543417cb615 Cr-Commit-Position: refs/heads/master@{#40348} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [debug] [reland] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203, chromium:656622 Committed: https://crrev.com/ce32e2ffd835062d764f3c0ee6a32543417cb615 Cr-Commit-Position: refs/heads/master@{#40348} ========== to ========== [debug] [reland] Consistently use script from FrameMirror ... instead of getting it from the FunctionMirror. For WASM frames (including asm.js -> WASM), the function is either unresolved or does not contain the script. The added test case failed before this CL. R=kozyatinskiy@chromium.org, yangguo@chromium.org, titzer@chromium.org BUG=v8:4203, chromium:656622 Committed: https://crrev.com/ce32e2ffd835062d764f3c0ee6a32543417cb615 Committed: https://crrev.com/ea51b8a764a80f047979e577c9d5945f0c83f362 Cr-Original-Commit-Position: refs/heads/master@{#40348} Cr-Commit-Position: refs/heads/master@{#40387} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ea51b8a764a80f047979e577c9d5945f0c83f362 Cr-Commit-Position: refs/heads/master@{#40387} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
