Bug 152294 – Web Inspector: Parse InjectedScriptSource as a built-in to get guaranteed non-user-overriden JSC built-ins

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* 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

[Radar WebKit Bug Importer]

<rdar://problem/23895760>

[Blaze Burg]

This would be awesome. Are you going to put up a patch?

[Timothy Hatcher]

*** Bug 154403 has been marked as a duplicate of this bug. ***

[Patrick Angle]

Created attachment 458022 [details]
WIP v0.1

[Patrick Angle]

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.

[Patrick Angle]

Created attachment 458023 [details]
WIP v0.2

[Patrick Angle]

Created attachment 458033 [details]
WIP v0.3

[Patrick Angle]

Created attachment 458104 [details]
WIP v0.4

[Patrick Angle]

Created attachment 458193 [details]
WIP v0.5

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Patrick Angle]

Created attachment 458228 [details]
WIP v0.6

[Patrick Angle]

Created attachment 458236 [details]
WIP v0.7

[Patrick Angle]

Created attachment 458308 [details]
WIP v0.8

[Patrick Angle]

Created attachment 458389 [details]
Patch v1.0

[Devin Rousso]

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

[Patrick Angle]

Created attachment 458460 [details]
Patch v1.1 - More @s, more tests

[Patrick Angle]

Created attachment 458489 [details]
Patch v1.2

[Patrick Angle]

Created attachment 458490 [details]
Patch v1.2

[Patrick Angle]

Created attachment 458754 [details]
Patch v1.3

[Devin Rousso]

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)

[Joseph Pecoraro]

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",
    …
);
```

[Patrick Angle]

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!

[Patrick Angle]

Created attachment 458769 [details]
Patch v1.4 - Review nits and notes

[Patrick Angle]

Created attachment 458798 [details]
Patch

[Patrick Angle]

Created attachment 458802 [details]
Patch

[Patrick Angle]

Created attachment 458813 [details]
Patch v1.7 - For Windows EWS

[Patrick Angle]

Created attachment 458825 [details]
Patch v1.8 - For Windows EWS

[Patrick Angle]

Created attachment 458826 [details]
Patch v1.8.1 - For Windows EWS

[Patrick Angle]

Created attachment 458840 [details]
Patch v1.8.2 - For Windows EWS

[Patrick Angle]

Created attachment 458877 [details]
Patch v1.9 - Only a LinkTimeConstant

[Patrick Angle]

Created attachment 458928 [details]
Patch v1.10 - This seems to actually pass the tests on Windows - still needs cleanup

[Patrick Angle]

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.

[Patrick Angle]

Created attachment 458971 [details]
Patch v2.0 - Rebase of v1.4

[Patrick Angle]

Created attachment 459031 [details]
Patch v2.1 - With Windows build fix that prevents using a stale WebCoreJSBuiltinInternals.h for dependent targets

[Patrick Angle]

(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.

[Patrick Angle]

Created attachment 459174 [details]
Patch v2.2 - With WIP build fix from bug 240297

[Patrick Angle]

Created attachment 459176 [details]
Patch v2.2.1 - With WIP build fix from bug 240297

[Patrick Angle]

Created attachment 459181 [details]
Patch v2.3 - For landing

[Patrick Angle]

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.

[EWS]

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].