[webkit-reviews] review granted: [Bug 152294] Web Inspector: Parse InjectedScriptSource as a built-in to get guaranteed non-user-overriden JSC built-ins : [Attachment 458754] Patch v1.3

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 152294: Web Inspector: Parse InjectedScriptSource as a built-in to get
guaranteed non-user-overriden JSC built-ins
https://bugs.webkit.org/show_bug.cgi?id=152294

Attachment 458754: Patch v1.3

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




--- Comment #21 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 458754
  --> https://bugs.webkit.org/attachment.cgi?id=458754
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. at getOwnPropertySymbols) {

NIT: I dont think we need this check anymore ��

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

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

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

ditto (:912)

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

ditto (:912)

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1357
> +	   preview.entries = entries. at 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)