Title: [286412] trunk/Source/WebInspectorUI
- Revision
- 286412
- Author
- pan...@apple.com
- Date
- 2021-12-01 21:03:13 -0800 (Wed, 01 Dec 2021)
Log Message
Web Inspector: Console execution context can become an unexpected selection on refresh/navigation
https://bugs.webkit.org/show_bug.cgi?id=233349
Reviewed by Devin Rousso.
There are really three bugs here in `WI.QuickConsole.prototype._handleFrameExecutionContextsCleared`, each of
which contribute to unexpected execution context selections.
1. If the frame of the active execution context commits a provisional load, our code attempts to re-select 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. This is resolved by no longer following a frame's execution
context on navigation if the current selection is `Auto` (tracked by the `_useExecutionContextOfInspectedNode`
variable).
2. Because of the very short timeframe for the above to take place before bailing (previously 10ms) there are
many situations where other large payloads over the protocol, like a complex DOM tree, will cause the timeout to
fire 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. This is improved by increasing the timeout to a still-brisk, but more
reasonable 100ms.
3. The timeout in (2) can lead to us having a selected execution context that may no longer be valid because the
fail-safe 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 drop down menu with actual contexts that are selectable) and are unable
to evaluate anything in the console until re-selecting an execution context.
Additional drive-by change to move the checks for whether or not we can use the execution context of a selected
node into its own method.
* UserInterface/Views/QuickConsole.js:
(WI.QuickConsole.prototype._populateActiveExecutionContextNavigationItemContextMenu):
(WI.QuickConsole.prototype._handleEngineeringShowInternalExecutionContextsSettingChanged):
(WI.QuickConsole.prototype._handleFrameExecutionContextsCleared):
(WI.QuickConsole.prototype._handleTargetRemoved):
(WI.QuickConsole.prototype._canUseExecutionContextOfInspectedNode):
Modified Paths
Diff
Modified: trunk/Source/WebInspectorUI/ChangeLog (286411 => 286412)
--- trunk/Source/WebInspectorUI/ChangeLog 2021-12-02 04:58:54 UTC (rev 286411)
+++ trunk/Source/WebInspectorUI/ChangeLog 2021-12-02 05:03:13 UTC (rev 286412)
@@ -1,3 +1,42 @@
+2021-12-01 Patrick Angle <pan...@apple.com>
+
+ Web Inspector: Console execution context can become an unexpected selection on refresh/navigation
+ https://bugs.webkit.org/show_bug.cgi?id=233349
+
+ Reviewed by Devin Rousso.
+
+ There are really three bugs here in `WI.QuickConsole.prototype._handleFrameExecutionContextsCleared`, each of
+ which contribute to unexpected execution context selections.
+
+ 1. If the frame of the active execution context commits a provisional load, our code attempts to re-select 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. This is resolved by no longer following a frame's execution
+ context on navigation if the current selection is `Auto` (tracked by the `_useExecutionContextOfInspectedNode`
+ variable).
+
+ 2. Because of the very short timeframe for the above to take place before bailing (previously 10ms) there are
+ many situations where other large payloads over the protocol, like a complex DOM tree, will cause the timeout to
+ fire 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. This is improved by increasing the timeout to a still-brisk, but more
+ reasonable 100ms.
+
+ 3. The timeout in (2) can lead to us having a selected execution context that may no longer be valid because the
+ fail-safe 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 drop down menu with actual contexts that are selectable) and are unable
+ to evaluate anything in the console until re-selecting an execution context.
+
+ Additional drive-by change to move the checks for whether or not we can use the execution context of a selected
+ node into its own method.
+
+ * UserInterface/Views/QuickConsole.js:
+ (WI.QuickConsole.prototype._populateActiveExecutionContextNavigationItemContextMenu):
+ (WI.QuickConsole.prototype._handleEngineeringShowInternalExecutionContextsSettingChanged):
+ (WI.QuickConsole.prototype._handleFrameExecutionContextsCleared):
+ (WI.QuickConsole.prototype._handleTargetRemoved):
+ (WI.QuickConsole.prototype._canUseExecutionContextOfInspectedNode):
+
2021-11-30 BJ Burg <bb...@apple.com>
Web Inspector: add ExtensionTabActivation diagnostic event
Modified: trunk/Source/WebInspectorUI/UserInterface/Views/QuickConsole.js (286411 => 286412)
--- trunk/Source/WebInspectorUI/UserInterface/Views/QuickConsole.js 2021-12-02 04:58:54 UTC (rev 286411)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/QuickConsole.js 2021-12-02 05:03:13 UTC (rev 286412)
@@ -33,7 +33,7 @@
this._toggleOrFocusKeyboardShortcut.implicitlyPreventsDefault = false;
this._keyboardShortcutDisabled = false;
- this._useExecutionContextOfInspectedNode = InspectorBackend.hasDomain("DOM");
+ this._useExecutionContextOfInspectedNode = this._canUseExecutionContextOfInspectedNode();
this._restoreSelectedExecutionContextForFrame = null;
this.element.classList.add("quick-console");
@@ -243,7 +243,7 @@
let activeExecutionContext = WI.runtimeManager.activeExecutionContext;
- if (InspectorBackend.hasDomain("DOM")) {
+ if (this._canUseExecutionContextOfInspectedNode()) {
let executionContextForInspectedNode = this._resolveDesiredActiveExecutionContext(true);
contextMenu.appendCheckboxItem(WI.UIString("Auto \u2014 %s").format(this._displayNameForExecutionContext(executionContextForInspectedNode, maxLength)), () => {
this._useExecutionContextOfInspectedNode = true;
@@ -356,7 +356,7 @@
if (WI.runtimeManager.activeExecutionContext.type !== WI.ExecutionContext.Type.Internal)
return;
- this._useExecutionContextOfInspectedNode = InspectorBackend.hasDomain("DOM");
+ this._useExecutionContextOfInspectedNode = this._canUseExecutionContextOfInspectedNode();
this._setActiveExecutionContext(this._resolveDesiredActiveExecutionContext());
}
@@ -388,20 +388,24 @@
return;
}
- // If this frame is navigating and it is selected in the UI we want to reselect its new item after navigation.
- if (committingProvisionalLoad && !this._restoreSelectedExecutionContextForFrame) {
+ // If this frame is navigating and it is selected in the UI we want to reselect its new item after navigation,
+ // however when `_useExecutionContextOfInspectedNode` is true, we should keep the execution context set to `Auto`.
+ if (committingProvisionalLoad && !this._restoreSelectedExecutionContextForFrame && !this._useExecutionContextOfInspectedNode) {
this._restoreSelectedExecutionContextForFrame = event.target;
// As a fail safe, if the frame never gets an execution context, clear the restore value.
setTimeout(() => {
- if (this._restoreSelectedExecutionContextForFrame)
- this._updateActiveExecutionContextDisplay();
+ if (!this._restoreSelectedExecutionContextForFrame)
+ return;
this._restoreSelectedExecutionContextForFrame = null;
- }, 10);
+
+ this._useExecutionContextOfInspectedNode = this._canUseExecutionContextOfInspectedNode();
+ this._setActiveExecutionContext(this._resolveDesiredActiveExecutionContext());
+ }, 100);
return;
}
- this._useExecutionContextOfInspectedNode = InspectorBackend.hasDomain("DOM");
+ this._useExecutionContextOfInspectedNode = this._canUseExecutionContextOfInspectedNode();
this._setActiveExecutionContext(this._resolveDesiredActiveExecutionContext());
}
@@ -428,7 +432,7 @@
return;
}
- this._useExecutionContextOfInspectedNode = InspectorBackend.hasDomain("DOM");
+ this._useExecutionContextOfInspectedNode = this._canUseExecutionContextOfInspectedNode();
this._setActiveExecutionContext(this._resolveDesiredActiveExecutionContext());
}
@@ -440,6 +444,11 @@
this._setActiveExecutionContext(this._resolveDesiredActiveExecutionContext());
}
+ _canUseExecutionContextOfInspectedNode()
+ {
+ return InspectorBackend.hasDomain("DOM");
+ }
+
_toggleOrFocus(event)
{
if (this._keyboardShortcutDisabled)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
