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.
Created attachment 313593 [details] [PATCH] Proposed Fix
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.
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.
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}?
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.
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.
<https://trac.webkit.org/changeset/218718/webkit>