Bug 195834 – Can't use $0, $1 etc when inspecting Google Docs pages because the content uses these for function names

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

[Simon Fraser (smfr)]

Dumping $0, $1 etc in the console when inspecting Google Docs pages dumps some function from the page, rather than the selected element.

Can we do something to keep console $ things separate from page $ things?

[Devin Rousso]

No matter what prefix (e.g. `$`) we pick, there's always a chance that the inspected page (or whatever the current context is) overrides it.  A particularly malicious website could decide to blanket create $0...$99 as empty values on `window` to entirely prevent Web Inspector from being able to save values.

One idea I had was to create a way for the developer to be able to customize the prefix from the Web Inspector frontend (e.g. an <input> in the Settings tab), so that if they notice that there are already existing values on the page, they could change `$` to be `$sadface` and avoid the collision/shadowing.  This would 100% require backend support, but it would be a somewhat easy change to make.  The discoverability of this isn't any worse than any other setting, but telling the "story" of this method would be much harder, specifically with the amount of space we traditionally have for any given setting in the Settings tab (e.g. one line with only a few words).

I like this approach as it gives the most flexibility to the developer (some people would prefer a different prefix, like `_` or `temp` or `joe`).

Alternatively (or in addition to), if we notice that a saved value (e.g. `$1`) would collide with an existing value on the page, we could surface a warning in the console suggesting that the developer change their prefix, or even try to do some smart "expansion" and add as many `_` as needed until there's no collision.

[Simon Fraser (smfr)]

I suggested that if you right-click and "Log Element", inspector finds the first unused $ value that it can, and uses that.

[Devin Rousso]

Created attachment 368402 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 368403 [details]
Patch

[EWS Watchlist]

Comment on attachment 368403 [details]
Patch

Attachment 368403 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12017008

New failing tests:
http/tests/inspector/dom/cross-domain-inspected-node-access.html
inspector/console/command-line-api.html

[EWS Watchlist]

Created attachment 368407 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 368403 [details]
Patch

Attachment 368403 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12017653

New failing tests:
http/tests/inspector/dom/cross-domain-inspected-node-access.html
inspector/console/command-line-api.html

[EWS Watchlist]

Created attachment 368409 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 368403 [details]
Patch

Attachment 368403 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12018022

New failing tests:
http/tests/inspector/dom/cross-domain-inspected-node-access.html
inspector/console/command-line-api.html

[EWS Watchlist]

Created attachment 368413 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 368415 [details]
Patch

[Devin Rousso]

Comment on attachment 368415 [details]
Patch

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

> Source/WebCore/inspector/CommandLineAPIModuleSource.js:58
> +    this.__defineGetter__(prefix("0"), bind(commandLineAPIImpl._inspectedObject, commandLineAPIImpl));

We should probably also have `$0` in addition to the prefixed version for compatibility.

[Devin Rousso]

Created attachment 372824 [details]
Patch

After talking offline with @JoePeck, we decided that a better course of action is to have the setting be an alias instead, meaning that both the input value and "$" work as a prefix.  This way, the existing functionality of "$" is preserved while still providing an alternate way of referencing the values that is less likely to collide with a value in the page.

[Devin Rousso]

Comment on attachment 372824 [details]
Patch

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

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:-1462
> -        delete this.$event;

Since the command line API is recreated each time something is entered into the Console, there's no need to call `delete` as `this` is a brand new object.

[EWS Watchlist]

Comment on attachment 372824 [details]
Patch

Attachment 372824 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12569288

New failing tests:
inspector/debugger/command-line-api-exception.html
inspector/debugger/command-line-api-exception-nested-catch.html

[EWS Watchlist]

Created attachment 372827 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 372824 [details]
Patch

Attachment 372824 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12569431

New failing tests:
inspector/debugger/command-line-api-exception.html
inspector/debugger/command-line-api-exception-nested-catch.html

[EWS Watchlist]

Created attachment 372830 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 372824 [details]
Patch

Attachment 372824 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12569448

New failing tests:
inspector/debugger/command-line-api-exception.html
inspector/debugger/command-line-api-exception-nested-catch.html

[EWS Watchlist]

Created attachment 372832 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 372824 [details]
Patch

Attachment 372824 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12569809

New failing tests:
storage/indexeddb/index-cursor.html
css3/filters/blur-various-radii.html

[EWS Watchlist]

Created attachment 372835 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Devin Rousso]

Created attachment 372839 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 372839 [details]
Patch

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

r=me

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1459
> +        this.$_ = injectedScript._lastResult;

I wonder if these should be `__defineGetter__` as well. It doesn't really matter, because they get recreated every evaluation, but I suspect it would throw errors when/if folks try to assign to them. That should have its own test if we make such a change.

> Source/JavaScriptCore/inspector/protocol/Runtime.json:322
> +            "description": "Creates an additional reference to all saved values in the Console by replacing the default $ prefix with the given string.",

This is not `replacing` but providing a parallel prefix. It seems more like a `setSavedResultPrefixAlias` but I'm fine with the existing naming.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:334
> +        WI.settings.consoleSavedResultAlias.addEventListener(WI.Setting.Event.Changed, updateSavedVariableText);

These event handlers need to get removed somewhere, or else they will exist forever on the global Setting.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:683
> +        WI.settings.consoleSavedResultAlias.addEventListener(WI.Setting.Event.Changed, (event) => {

These event handlers need to get removed somewhere, or else they will exist forever on the global Setting.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:275
> +            consoleSavedResultAliasInput.addEventListener("keydown", (event) => {
> +                if (!/[a-zA-Z0-9_$]/.test(event.key) || (consoleSavedResultAliasInput.selectionStart === 0 && /[0-9]/.test(event.key))) {
> +                    event.preventDefault();
> +                    InspectorFrontendHost.beep();
> +                }
> +            });

I guess this validation doesn't need to happen on the backend because this is actually becoming a property, not a variable name.

Maybe special case if the final value is "$" it could clear it.

> LayoutTests/inspector/runtime/setSavedResultAlias.html:46
> +            InspectorTest.newline();

This would totally have made sense as 3 tests instead of just newlines.

[Devin Rousso]

Comment on attachment 372839 [details]
Patch

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

>> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1459
>> +        this.$_ = injectedScript._lastResult;
> 
> I wonder if these should be `__defineGetter__` as well. It doesn't really matter, because they get recreated every evaluation, but I suspect it would throw errors when/if folks try to assign to them. That should have its own test if we make such a change.

I like it!

>> Source/JavaScriptCore/inspector/protocol/Runtime.json:322
>> +            "description": "Creates an additional reference to all saved values in the Console by replacing the default $ prefix with the given string.",
> 
> This is not `replacing` but providing a parallel prefix. It seems more like a `setSavedResultPrefixAlias` but I'm fine with the existing naming.

Ah oops.  This was from an earlier version.

>> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:334
>> +        WI.settings.consoleSavedResultAlias.addEventListener(WI.Setting.Event.Changed, updateSavedVariableText);
> 
> These event handlers need to get removed somewhere, or else they will exist forever on the global Setting.

Hmmmm... I'll have to think of a way around this.

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:275
>> +            });
> 
> I guess this validation doesn't need to happen on the backend because this is actually becoming a property, not a variable name.
> 
> Maybe special case if the final value is "$" it could clear it.

Good call!

>> LayoutTests/inspector/runtime/setSavedResultAlias.html:46
>> +            InspectorTest.newline();
> 
> This would totally have made sense as 3 tests instead of just newlines.

I'd rather not, as that would mean that each test relies on the state set by the previous test.  I prefer keeping test cases completely independent.

[Devin Rousso]

Created attachment 375579 [details]
Patch

[Devin Rousso]

Created attachment 375582 [details]
Patch

The placeholder should be "$" to match the default prefix.

[EWS Watchlist]

Comment on attachment 375582 [details]
Patch

Attachment 375582 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12866317

New failing tests:
http/tests/inspector/dom/cross-domain-inspected-node-access.html
inspector/console/command-line-api.html

[EWS Watchlist]

Created attachment 375589 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 375594 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375594 [details]
Patch

Clearing flags on attachment: 375594

Committed r248287: <https://trac.webkit.org/changeset/248287>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53970472>