Bug 173698 – Web Inspector: Pausing with a deep call stack can be very slow, avoid eagerly generating object previews

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Pausing with a deep call stack can be very slow, avoid eagerly generating object previews.

Test:
<script>
var count = 0;
function f() {
    if (++count > 1000)
        throw new Error;
    f();
}
f();
</script>

Steps to Reproduce:
1. Inspect attached test page
2. Enable Pause on Uncaught Exceptions
3. Reload the page (triggers the exception)
  => Pause, takes a very long time to show debugger UI

Notes:
We spend about 95% of the time in the backend generating object previews for each of the 1000 CallFrame's `thisObject`. Which is the `window` in this example. We can avoid generating the ObjectPreview eagerly (here), and instead generate them lazily if we ever need to see them in the frontend.

[Joseph Pecoraro]

Created attachment 313593 [details]
[PATCH] Proposed Fix

[Build Bot]

Attachment 313593 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.h:65:  The parameter name "preview" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Matt Baker]

Comment on attachment 313593 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=313593&action=review

> LayoutTests/ChangeLog:10
> +        Test the new protocol commnand `Runtime.getPreview` as well as the frontend

commnand -> command

> LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:12
> +    

Remove whitespace.

[Matt Baker]

Comment on attachment 313593 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=313593&action=review

I've gotten through the tests, will continue with backend/frontend.

> LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:51
> +            if (callFrame.thisObject.canLoadPreview()) {

Should we also assert that callFrame.thisObject.canLoadPreview()? Later it's assumed that every call frame was mapped to a Promise, and access callFrame.thisObject.preview for every frame.

> LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:70
> +                InspectorTest.expectThat(expectedFrame.thisValue[0] === prop.name, `'this' value should have expected property: ${expectedFrame.thisValue[0]}`);

Could use InspectorTest.expectEqual here.

> LayoutTests/inspector/runtime/getPreview-expected.txt:28
> +PASS: RemoteObject.updatePreview should return null for a object RemoteObject.

This last result seemed strange at first, until reading the test code and seeing that it's the fake remote object.

> LayoutTests/inspector/runtime/getPreview-expected.txt:46
> +Error: Could not find InjectedScript for objectId

Nit: include a period at end of sentence, to match the lines above.

> LayoutTests/inspector/runtime/getPreview.html:8
> +    InspectorTest.debug();

Remove InspectorTest.debug();

> LayoutTests/inspector/runtime/getPreview.html:60
> +            function testRemoteObject(remoteObject, type) {

`type` isn't used. Was it meant to be included in test output instead of ${remoteObject.type}?

[Joseph Pecoraro]

Comment on attachment 313593 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=313593&action=review

>> LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:51
>> +            if (callFrame.thisObject.canLoadPreview()) {
> 
> Should we also assert that callFrame.thisObject.canLoadPreview()? Later it's assumed that every call frame was mapped to a Promise, and access callFrame.thisObject.preview for every frame.

Nope. The `thisObject` of a CallFrame could be null, or a primitive value, and I believe that is the case in this test.

I'm not sure what you mean by call frame's being mapped to promises. Here we are calling `updatePreview` only on a few call frames, and wait for all of those requests to finish (having updated the RemoteObjects in place) before proceeding to loop over all call frames.

>> LayoutTests/inspector/runtime/getPreview-expected.txt:28
>> +PASS: RemoteObject.updatePreview should return null for a object RemoteObject.
> 
> This last result seemed strange at first, until reading the test code and seeing that it's the fake remote object.

Ahh yea, I'll use that extra `type` argument to clarify this case!

>> LayoutTests/inspector/runtime/getPreview-expected.txt:46
>> +Error: Could not find InjectedScript for objectId
> 
> Nit: include a period at end of sentence, to match the lines above.

This is just logging the exact protocol `error` value.

[Matt Baker]

Comment on attachment 313593 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=313593&action=review

Nice work, r=me with a few nits/comments.

> Source/JavaScriptCore/ChangeLog:10
> +        the `thisObject` of each of the call frame's. In some cases, this could be more

No apostrophe in frames.

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:240
> +        var object = this._objectForId(parsedObjectId);

Nit: The rest of the file still uses var, but why not use let for new code?

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:475
> +        var result = evalFunction.call(object, expression, commandLineAPI);

Nit: let

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:170
> +    muteConsole();

This is such a common pattern in InspectorRuntimeAgent, I wonder if it would be worth making into a RAII-style guard.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/218718/webkit>