Bug 195834 - Can't use $0, $1 etc when inspecting Google Docs pages because the content uses these for function names
Summary: Can't use $0, $1 etc when inspecting Google Docs pages because the content us...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-15 16:23 PDT by Simon Fraser (smfr)
Modified: 2019-08-05 20:38 PDT (History)
14 users (show)

See Also:


Attachments
Patch (43.19 KB, patch)
2019-04-27 10:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (43.57 KB, patch)
2019-04-27 10:40 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.73 MB, application/zip)
2019-04-27 11:58 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-highsierra (3.08 MB, application/zip)
2019-04-27 13:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.88 MB, application/zip)
2019-04-27 14:44 PDT, Build Bot
no flags Details
Patch (45.69 KB, patch)
2019-04-27 16:46 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (44.36 KB, patch)
2019-06-25 01:02 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.32 MB, application/zip)
2019-06-25 01:55 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.75 MB, application/zip)
2019-06-25 02:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (3.00 MB, application/zip)
2019-06-25 02:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews211 for win-future (13.85 MB, application/zip)
2019-06-25 05:00 PDT, Build Bot
no flags Details
Patch (50.12 KB, patch)
2019-06-25 09:53 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (52.32 KB, patch)
2019-08-05 16:33 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (52.31 KB, patch)
2019-08-05 16:37 PDT, Devin Rousso
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (3.48 MB, application/zip)
2019-08-05 17:40 PDT, Build Bot
no flags Details
Patch (52.33 KB, patch)
2019-08-05 19:20 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-03-15 16:23:17 PDT
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?
Comment 1 Devin Rousso 2019-04-26 15:31:24 PDT
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.
Comment 2 Simon Fraser (smfr) 2019-04-26 15:53:25 PDT
I suggested that if you right-click and "Log Element", inspector finds the first unused $ value that it can, and uses that.
Comment 3 Devin Rousso 2019-04-27 10:32:21 PDT
Created attachment 368402 [details]
Patch
Comment 4 Build Bot 2019-04-27 10:33:47 PDT Comment hidden (obsolete)
Comment 5 Devin Rousso 2019-04-27 10:40:02 PDT
Created attachment 368403 [details]
Patch
Comment 6 Build Bot 2019-04-27 11:58:06 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2019-04-27 11:58:08 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2019-04-27 13:06:49 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2019-04-27 13:06:51 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2019-04-27 14:44:57 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2019-04-27 14:44:59 PDT Comment hidden (obsolete)
Comment 12 Devin Rousso 2019-04-27 16:46:51 PDT
Created attachment 368415 [details]
Patch
Comment 13 Devin Rousso 2019-06-14 18:24:39 PDT
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.
Comment 14 Devin Rousso 2019-06-25 01:02:53 PDT
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.
Comment 15 Devin Rousso 2019-06-25 01:03:53 PDT
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.
Comment 16 Build Bot 2019-06-25 01:54:58 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2019-06-25 01:55:00 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2019-06-25 02:23:08 PDT Comment hidden (obsolete)
Comment 19 Build Bot 2019-06-25 02:23:10 PDT Comment hidden (obsolete)
Comment 20 Build Bot 2019-06-25 02:53:29 PDT Comment hidden (obsolete)
Comment 21 Build Bot 2019-06-25 02:53:32 PDT Comment hidden (obsolete)
Comment 22 Build Bot 2019-06-25 05:00:09 PDT Comment hidden (obsolete)
Comment 23 Build Bot 2019-06-25 05:00:12 PDT Comment hidden (obsolete)
Comment 24 Devin Rousso 2019-06-25 09:53:52 PDT
Created attachment 372839 [details]
Patch
Comment 25 Joseph Pecoraro 2019-08-05 13:11:27 PDT
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.
Comment 26 Devin Rousso 2019-08-05 15:45:28 PDT
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.
Comment 27 Devin Rousso 2019-08-05 16:33:25 PDT
Created attachment 375579 [details]
Patch
Comment 28 Devin Rousso 2019-08-05 16:37:08 PDT
Created attachment 375582 [details]
Patch

The placeholder should be "$" to match the default prefix.
Comment 29 Build Bot 2019-08-05 17:40:03 PDT Comment hidden (obsolete)
Comment 30 Build Bot 2019-08-05 17:40:05 PDT Comment hidden (obsolete)
Comment 31 Devin Rousso 2019-08-05 19:20:59 PDT
Created attachment 375594 [details]
Patch
Comment 32 WebKit Commit Bot 2019-08-05 20:37:50 PDT
Comment on attachment 375594 [details]
Patch

Clearing flags on attachment: 375594

Committed r248287: <https://trac.webkit.org/changeset/248287>
Comment 33 WebKit Commit Bot 2019-08-05 20:37:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Radar WebKit Bug Importer 2019-08-05 20:38:35 PDT
<rdar://problem/53970472>