Bug 152294 - Web Inspector: Parse InjectedScriptSource as a built-in to get guaranteed non-user-overriden JSC built-ins
Summary: Web Inspector: Parse InjectedScriptSource as a built-in to get guaranteed non...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Patrick Angle
URL:
Keywords: InRadar
: 154403 (view as bug list)
Depends on:
Blocks: 239774
  Show dependency treegraph
 
Reported: 2015-12-14 21:35 PST by Joseph Pecoraro
Modified: 2022-05-11 17:33 PDT (History)
25 users (show)

See Also:


Attachments
WIP v0.1 (182.24 KB, patch)
2022-04-20 17:47 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
WIP v0.2 (180.57 KB, patch)
2022-04-20 17:52 PDT, Patrick Angle
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP v0.3 (180.88 KB, patch)
2022-04-20 20:15 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
WIP v0.4 (75.79 KB, patch)
2022-04-21 17:36 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
WIP v0.5 (89.37 KB, patch)
2022-04-22 18:41 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
WIP v0.6 (103.25 KB, patch)
2022-04-23 23:34 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
WIP v0.7 (103.89 KB, patch)
2022-04-24 13:01 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
WIP v0.8 (109.78 KB, patch)
2022-04-25 16:29 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.0 (99.19 KB, patch)
2022-04-26 12:42 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.1 - More @s, more tests (131.86 KB, patch)
2022-04-27 12:17 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.2 (143.11 KB, patch)
2022-04-27 21:18 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.2 (143.08 KB, patch)
2022-04-27 21:20 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.3 (150.15 KB, patch)
2022-05-03 13:30 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.4 - Review nits and notes (153.07 KB, patch)
2022-05-03 19:02 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch (153.49 KB, patch)
2022-05-04 08:33 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch (153.43 KB, patch)
2022-05-04 08:53 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.7 - For Windows EWS (153.21 KB, patch)
2022-05-04 11:04 PDT, Patrick Angle
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch v1.8 - For Windows EWS (154.22 KB, patch)
2022-05-04 13:55 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.8.1 - For Windows EWS (152.78 KB, patch)
2022-05-04 14:34 PDT, Patrick Angle
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch v1.8.2 - For Windows EWS (153.15 KB, patch)
2022-05-04 18:51 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.9 - Only a LinkTimeConstant (4.28 KB, patch)
2022-05-05 09:02 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.10 - This seems to actually pass the tests on Windows - still needs cleanup (151.76 KB, patch)
2022-05-05 17:25 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v2.0 - Rebase of v1.4 (153.45 KB, patch)
2022-05-06 13:51 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v2.1 - With Windows build fix that prevents using a stale WebCoreJSBuiltinInternals.h for dependent targets (155.02 KB, patch)
2022-05-09 00:03 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v2.2 - With WIP build fix from bug 240297 (153.86 KB, patch)
2022-05-11 11:33 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v2.2.1 - With WIP build fix from bug 240297 (153.85 KB, patch)
2022-05-11 11:48 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v2.3 - For landing (153.44 KB, patch)
2022-05-11 13:39 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-12-14 21:35:35 PST
* SUMMARY
Parse InjectedScriptSource as a built-in to get guaranteed non-user-overriden built-ins.

During debugging, InjectedScriptSource.js gets evaluated in the main context of each page / JSGlobalContext. Because of this it must be careful to avoid user overridable functions that may not behave as expected.

For example instead it cannot just use `Object` because that is `global.Object` and can be overridden by the page to do whatever it wants, like `global.Object = function() { throw "Error"; }`.

There are many functions which we cannot "avoid" using, but cannot be guaranteed that they aren't overriden by the user.

    Object, Array, String
    Object.getOwnPropertyNames
    Object.getOwnPropertyDescriptor
    Object.getOwnPropertySymbols
    Object.prototype.hasOwnProperty
    Object.prototype.__defineGetter__
    Function.prototype.call
    Element.prototype.hasAttribute

There are plenty we could re-implement ourselves, but would mostly be a waste:

    Set
    String.prototype.trim
    String.prototype.replace

Instead, we should just use the built-in logic in which case we can guarantee we are using the built-in versions of these:

    @Set
    @Object.@getOwnPropertySymbols
    ...

It might get a bit tricky as we move to WebCore accessors (Element.prototype.hasAttribute), but we can fix the majority of brittle code here by using builtins.

* TEST
<script>
window.Set = function() { throw "Error"; }
</script>

* STEPS TO REPRODUCE
1. Inspect test page
2. js> window
3. Expand window object in the console
  => ASSERT in debug builds, No properties in Release builds
Comment 1 Radar WebKit Bug Importer 2015-12-14 21:36:08 PST
<rdar://problem/23895760>
Comment 2 BJ Burg 2015-12-15 09:07:21 PST
This would be awesome. Are you going to put up a patch?
Comment 3 Timothy Hatcher 2016-02-18 12:27:23 PST
*** Bug 154403 has been marked as a duplicate of this bug. ***
Comment 4 Patrick Angle 2022-04-20 17:47:01 PDT
Created attachment 458022 [details]
WIP v0.1
Comment 5 Patrick Angle 2022-04-20 17:47:53 PDT
Comment on attachment 458022 [details]
WIP v0.1

Not intended for review yet - trying to gauge if/how many tests are broken in various corners of the test suite.
Comment 6 Patrick Angle 2022-04-20 17:52:06 PDT
Created attachment 458023 [details]
WIP v0.2
Comment 7 Patrick Angle 2022-04-20 20:15:53 PDT
Created attachment 458033 [details]
WIP v0.3
Comment 8 Patrick Angle 2022-04-21 17:36:49 PDT
Created attachment 458104 [details]
WIP v0.4
Comment 9 Patrick Angle 2022-04-22 18:41:02 PDT
Created attachment 458193 [details]
WIP v0.5
Comment 10 Yusuke Suzuki 2022-04-22 21:30:58 PDT
Comment on attachment 458193 [details]
WIP v0.5

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

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1085
> +    auto* jsonProperty = JSONObject::create(vm, JSONObject::createStructure(vm, this, m_objectPrototype.get()));
> +    m_jsonProperty.set(vm, this, jsonProperty);
> +    m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::JSON)].set(vm, this, jsonProperty);

Let's not newly create JSONObject. Let's

1. Convert JSON object definition from PropertyCallback to CellProperty+LazyProperty
2. Define lazy init function to initialize it
3. Use that lazy initializer inside m_linkTimeConstants's initLater.
Comment 11 Yusuke Suzuki 2022-04-22 21:32:46 PDT
Comment on attachment 458193 [details]
WIP v0.5

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

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1085
>> +    m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::JSON)].set(vm, this, jsonProperty);
> 
> Let's not newly create JSONObject. Let's
> 
> 1. Convert JSON object definition from PropertyCallback to CellProperty+LazyProperty
> 2. Define lazy init function to initialize it
> 3. Use that lazy initializer inside m_linkTimeConstants's initLater.

Ah, but in this case, probably, just exposing JSON.stringify / JSON.parse directly. And share the functions with JSON object by defining them lazily in JSGlobalObject.
Anyway, we are not designing creating multiple JSONObject in one JSGlobalObject, so avoid it.
Comment 12 Patrick Angle 2022-04-23 23:34:58 PDT
Created attachment 458228 [details]
WIP v0.6
Comment 13 Patrick Angle 2022-04-24 13:01:58 PDT
Created attachment 458236 [details]
WIP v0.7
Comment 14 Patrick Angle 2022-04-25 16:29:54 PDT
Created attachment 458308 [details]
WIP v0.8
Comment 15 Patrick Angle 2022-04-26 12:42:25 PDT
Created attachment 458389 [details]
Patch v1.0
Comment 16 Devin Rousso 2022-04-26 13:24:29 PDT
Comment on attachment 458389 [details]
Patch v1.0

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

omg yes so awesome =D

as we discussed offline, i think there are still a few spots that need to be adjusted to use a builtin, but you got almost all of them :)

it'd be super nice if we could have a test that simulates a page overriding some of these builtins to confirm that this is working as expected (e.g. have a page override `Array.prototype.push = function() { didPush = true; }` and then confirm `!didPush` once the injected script logic has finished)

> Source/JavaScriptCore/Scripts/wkbuiltins/builtins_model.py:337
> +                # FIXME: <webkit.org/b/######> Regular expressions containing unbalanced quotation marks (like the one

oops?

> Source/JavaScriptCore/builtins/IteratorHelpers.js:64
> +    if (@isUndefinedOrNull(iteratorFunction))
> +        @throwTypeError("builtinIterable requires that the property `iterable[Symbol.iterator]` exists.");

NIT: is it really useful to distinguish between "not set" and "not callable"?

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1047
> +            return value[@@toStringTag];

NIT: can we do `value.@@toStringTag?`

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1065
> +        // FIXME: <webkit.org/b/239774> Injected script should use WebCore built-ins.

FWIW you could probably do this by having something similar to `_inspectObject` so that the WebCore builtins stay in WebCore :)

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1140
> +            properties: isTableRowsRequest ? 1000 : (5 > firstLevelKeysCount ? 5 : firstLevelKeysCount),
> +            indexes: isTableRowsRequest ? 1000 : (10 > firstLevelKeysCount ? 10 : firstLevelKeysCount),

Style: I'd flip the order so that the variable is first (and this way the constant numbers are closer together)

NIT: maybe we should make our `function max` along the lines of how we already have a `toString`?

> Source/WebCore/inspector/CommandLineAPIModule.cpp:56
> +    ASSERT_NOT_REACHED();

should this be a `RELEASE_ASSERT_NOT_REACHED`?

> LayoutTests/ChangeLog:12
> +        - The Injected script (and modules) will no longer appear as scripts in Web Inspector.

i wonder if we should find some other internal script (e.g. modern media controls) and use that as the thing we step inside
Comment 17 Patrick Angle 2022-04-27 12:17:51 PDT
Created attachment 458460 [details]
Patch v1.1 - More @s, more tests
Comment 18 Patrick Angle 2022-04-27 21:18:56 PDT
Created attachment 458489 [details]
Patch v1.2
Comment 19 Patrick Angle 2022-04-27 21:20:02 PDT
Created attachment 458490 [details]
Patch v1.2
Comment 20 Patrick Angle 2022-05-03 13:30:07 PDT
Created attachment 458754 [details]
Patch v1.3
Comment 21 Devin Rousso 2022-05-03 14:07:21 PDT
Comment on attachment 458754 [details]
Patch v1.3

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

r=mews, awesome work!  Thanks for iterating :)

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:31
> +function createPrototypelessObject(/* key1, value1, key2, value2, ... */)

NIT: `createObjectWithoutPrototype`?

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:44
> +function createPrototypelessArray(/* value1, value2, ... */)

ditto (:31) `createArrayWithoutPrototype`

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:118
> +function createPrototypelessArrayFromArguments(argumentsObject) {

ditto (:31) `createArrayWithoutPrototypeFromArguments`

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:269
> +        return this._getProperties(objectId, collectionMode, @createPrototypelessObject("fetchStart", fetchStart, "fetchCount", fetchCount, "generatePreview", generatePreview));

NIT: I'd put this onto separate lines so that it's easier to read

ditto for every `@createObjectWithoutPrototype` that has more than two arguments (i.e. one property) below

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:811
> +            if (@Object.@getOwnPropertySymbols) {

NIT: I dont think we need this check anymore 🤔

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:912
> +        let existingIndex = this._savedResults.@indexOf(result);

will this actually work if it doesn't inherit from `Array.prototype`?

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1243
> +        if (firstLevelKeys && !firstLevelKeys.@includes(name))

ditto (:912)

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1347
> +            entries.@pop();

ditto (:912)

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1357
> +        preview.entries = entries.@map(function(entry) {

ditto (:912)

> LayoutTests/inspector/injected-script/observable.html:141
> +        propertyString: "\"push\"",

we may also wanna add tests for something like `Array.prototype.get 42` and/or `Object.prototype.get subtype` and/or etc. (i.e. find something in one of the objects we pass into functions just like `options = {}` and see what happens if you make `Object.prototype` have one of those properties) to make sure that `createObjectWithoutPrototype`/`createArrayWithoutPrototype` are doing the right thing (i.e. not allowing getters on the prototype to interfere with the injected script)
Comment 22 Joseph Pecoraro 2022-05-03 15:44:04 PDT
Comment on attachment 458754 [details]
Patch v1.3

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

Very cool!

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:46
> +    let array = new @Array();

Maybe `new @Array(arguments.length)`?

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1496
> +InjectedScript.CallFrameProxy._scopeTypeNames[0] = "global"; // GLOBAL_SCOPE

Was there an issue with the `@createPrototypelessObject` arguments convenience?

```
InjectedScript.CallFrameProxy._scopeTypeNames = @createPrototypelessObject(
    0, "global",
    1, "with",
    …
);
```
Comment 23 Patrick Angle 2022-05-03 17:14:23 PDT
Comment on attachment 458754 [details]
Patch v1.3

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

>> Source/JavaScriptCore/inspector/InjectedScriptSource.js:912
>> +        let existingIndex = this._savedResults.@indexOf(result);
> 
> will this actually work if it doesn't inherit from `Array.prototype`?

This works by accident, it seems. I need to `array.__proto__ = null;` in the create method.

>> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1496
>> +InjectedScript.CallFrameProxy._scopeTypeNames[0] = "global"; // GLOBAL_SCOPE
> 
> Was there an issue with the `@createPrototypelessObject` arguments convenience?
> 
> ```
> InjectedScript.CallFrameProxy._scopeTypeNames = @createPrototypelessObject(
>     0, "global",
>     1, "with",
>     …
> );
> ```

No issue - I had actually made this change at one point, but must have reverted it locally somehow!
Comment 24 Patrick Angle 2022-05-03 19:02:53 PDT
Created attachment 458769 [details]
Patch v1.4 - Review nits and notes
Comment 25 Patrick Angle 2022-05-04 08:33:31 PDT
Created attachment 458798 [details]
Patch
Comment 26 Patrick Angle 2022-05-04 08:53:40 PDT
Created attachment 458802 [details]
Patch
Comment 27 Patrick Angle 2022-05-04 11:04:50 PDT
Created attachment 458813 [details]
Patch v1.7 - For Windows EWS
Comment 28 Patrick Angle 2022-05-04 13:55:13 PDT
Created attachment 458825 [details]
Patch v1.8 - For Windows EWS
Comment 29 Patrick Angle 2022-05-04 14:34:21 PDT
Created attachment 458826 [details]
Patch v1.8.1 - For Windows EWS
Comment 30 Patrick Angle 2022-05-04 18:51:54 PDT
Created attachment 458840 [details]
Patch v1.8.2 - For Windows EWS
Comment 31 Patrick Angle 2022-05-05 09:02:35 PDT
Created attachment 458877 [details]
Patch v1.9 - Only a LinkTimeConstant
Comment 32 Patrick Angle 2022-05-05 17:25:32 PDT
Created attachment 458928 [details]
Patch v1.10 - This seems to actually pass the tests on Windows - still needs cleanup
Comment 33 Patrick Angle 2022-05-06 13:13:43 PDT
Windows test crashes have been narrowed down to something to do with building. If you build the patch twice back-to-back, the tests pass. I'm going to upload a rebased copy of this patch since it seems a few bits have gone stale since v1.4, which is the code that was intended to be landed.
Comment 34 Patrick Angle 2022-05-06 13:51:34 PDT
Created attachment 458971 [details]
Patch v2.0 - Rebase of v1.4
Comment 35 Patrick Angle 2022-05-09 00:03:01 PDT
Created attachment 459031 [details]
Patch v2.1 - With Windows build fix that prevents using a stale WebCoreJSBuiltinInternals.h for dependent targets
Comment 36 Patrick Angle 2022-05-09 00:05:02 PDT
(In reply to Patrick Angle from comment #35)
> Created attachment 459031 [details]
> Patch v2.1 - With Windows build fix that prevents using a stale
> WebCoreJSBuiltinInternals.h for dependent targets

Patch name is a bit misleading, really. This issue exists for all cmake platforms as best as I can tell, but only Windows is effected for testing because the other cmake platforms don't run their tests against WebKitLegacy/DumpRenderTree, which is where the stale header was being used.
Comment 37 Patrick Angle 2022-05-11 11:33:40 PDT
Created attachment 459174 [details]
Patch v2.2 - With WIP build fix from bug 240297
Comment 38 Patrick Angle 2022-05-11 11:48:45 PDT
Created attachment 459176 [details]
Patch v2.2.1 - With WIP build fix from bug 240297
Comment 39 Patrick Angle 2022-05-11 13:39:40 PDT
Created attachment 459181 [details]
Patch v2.3 - For landing
Comment 40 Patrick Angle 2022-05-11 13:42:51 PDT
AppleWin bot will fail tests after first build, and begin passing tests after the next incremental build - this is being addressed in bug 240297. The build fix in that bug was used to test v2.2.1 of this patch.
Comment 41 EWS 2022-05-11 17:33:07 PDT
Committed r294082 (250474@main): <https://commits.webkit.org/250474@main>

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