Bug 233349

Summary: Web Inspector: Console execution context can become an unexpected selection on refresh/navigation
Product: WebKit Reporter: Patrick Angle <pangle>
Component: Web InspectorAssignee: Patrick Angle <pangle>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, hi, inspector-bugzilla-changes, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1.0
none
Patch v1.1 - Address review nits none

Description Patrick Angle 2021-11-18 18:25:37 PST
<rdar://68229876>

Steps to (flakily) reproduce:
1. Go to https://books.apple.com/us/audiobook/winning-unabridged/id1561431722
2. Inspect the page.
3. Switch to the console tab *(explained below)
4. Confirm the console works with 1+1 or other similarly simple input.
5. Refresh the page.
6. Notice that sometime the work Auto disappears from the execution context selector, and the entire control turns blue indicating a non-Auto selection.

* This is /much/ harder to reproduce on the Elements tab, for example because there is currently a very short 10ms timeout during which the current frame's execution context will be eligible for re-selection post-load. Increasing the timeout in QuickConsole.prototype._handleFrameExecutionContextsCleared to 100ms makes this much easier to reproduce. This makes for two somewhat unfortunate bugs here:
1. If the frame of the active execution context commits a provisional load, our code attempts to reselect the new context for that frame, which results in clearing the `Auto` bit for selected context, which means the context no longer will follow the selected DOM node.
2. Because of the very short timeframe for the above to take place before bailing (currently 10ms) there are many situations where other large payloads over the protocol, like a complex DOM tree, will cause the time to expire before we receive an event telling us there is a new execution context for the frame, which means we end up not following the frame anyways... which leads to...
3. Which can lead to us having a selected execution context that may no longer be valid since the failsafe in QuickConsole.prototype._handleFrameExecutionContextsCleared doesn't really fail safely, since it doesn't provide a new execution context to be active and instead seems to rely on the hope that the context will still work for future invocations, which I believe is how many users are getting into the state of a blank execution context picker (with a working dropdown menu with actual contexts that are selectable) and are unable to evaluate anything in the console.
Comment 1 Patrick Angle 2021-11-18 18:34:52 PST
Created attachment 444767 [details]
Patch v1.0
Comment 2 Devin Rousso 2021-11-19 12:30:50 PST
Comment on attachment 444767 [details]
Patch v1.0

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

r=me, nice catch :)

> Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:400
>                  this._restoreSelectedExecutionContextForFrame = null;

Style: I'd add a newline after this

> Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:401
> +                this._useExecutionContextOfInspectedNode = InspectorBackend.hasDomain("DOM");

NIT: We should probably make a `_canUseExecutionContextOfInspectedNode()` or something since this is done so often.
Comment 3 Patrick Angle 2021-12-01 17:03:21 PST
Created attachment 445638 [details]
Patch v1.1 - Address review nits
Comment 4 EWS 2021-12-01 21:03:20 PST
Committed r286412 (244758@main): <https://commits.webkit.org/244758@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445638 [details].