* 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
<rdar://problem/23895760>
This would be awesome. Are you going to put up a patch?
*** Bug 154403 has been marked as a duplicate of this bug. ***
Created attachment 458022 [details] WIP v0.1
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.
Created attachment 458023 [details] WIP v0.2
Created attachment 458033 [details] WIP v0.3
Created attachment 458104 [details] WIP v0.4
Created attachment 458193 [details] WIP v0.5
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 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.
Created attachment 458228 [details] WIP v0.6
Created attachment 458236 [details] WIP v0.7
Created attachment 458308 [details] WIP v0.8
Created attachment 458389 [details] Patch v1.0
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
Created attachment 458460 [details] Patch v1.1 - More @s, more tests
Created attachment 458489 [details] Patch v1.2
Created attachment 458490 [details] Patch v1.2
Created attachment 458754 [details] Patch v1.3
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 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 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!
Created attachment 458769 [details] Patch v1.4 - Review nits and notes
Created attachment 458798 [details] Patch
Created attachment 458802 [details] Patch
Created attachment 458813 [details] Patch v1.7 - For Windows EWS
Created attachment 458825 [details] Patch v1.8 - For Windows EWS
Created attachment 458826 [details] Patch v1.8.1 - For Windows EWS
Created attachment 458840 [details] Patch v1.8.2 - For Windows EWS
Created attachment 458877 [details] Patch v1.9 - Only a LinkTimeConstant
Created attachment 458928 [details] Patch v1.10 - This seems to actually pass the tests on Windows - still needs cleanup
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.
Created attachment 458971 [details] Patch v2.0 - Rebase of v1.4
Created attachment 459031 [details] Patch v2.1 - With Windows build fix that prevents using a stale WebCoreJSBuiltinInternals.h for dependent targets
(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.
Created attachment 459174 [details] Patch v2.2 - With WIP build fix from bug 240297
Created attachment 459176 [details] Patch v2.2.1 - With WIP build fix from bug 240297
Created attachment 459181 [details] Patch v2.3 - For landing
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.
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].