1020609 - Implement Xrays to Array objects

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

This ended up being pretty orthogonal from the TypedArray work, so I decided to split it out from bug 987163.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=12e2c896f82a

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Make configurability check in Xray defineProperty match the spec. v1

This code is basically emulating the ES semantics with respect to non-configurable
properties. Non-configurable value properties can still be writable, in which case
their value and writability may be updated.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Inherit property descriptor attributes for pre-existing own value props in Proxy::set. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Fix up SpecialPowers for Array Xrays. v1

This just makes the special Object behavior apply to Array as well.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Implement Xrays to Arrays. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Fix up webconsole tests. v1

I'm somewhat skeptical that this is actually the right thing here. It seems
more likely that we in fact don't want the Xray behavior for Object/Array in
webconsole stringification. But the devtools actor stuff is complicated
enough that I'm not sure where that should go.

The basic setup is that Object (and now Array) Xrays apply various heuristics
to try to filter out dangerous properties from the underlying object. This
includes accessor properties, shadowing properties, callables, and non-Xrayable
objects.

The window.array2 test needs to be modified because now we now invoke toString
in chrome rather than in content, so we see XrayWrappers for the objects in the
array. This seems correct to me, and if it isn't, the devtools code should
probably be explicitly waiving Xrays.

The window.array3 test needs to be modified for the same reason as the above,
_and_ because we're not allowed to pull a callable property off of an xrayed
Object or Array (note the |,,| entry).

The window.map1 test needs to be modified because one of the entries is a Map,
which we don't have Xrays for, and so pulling it out of the Array-of-keys fails.
This is the fix that leads me most to be believe that the approach in this patch
isn't the right one.

Also, it would be nice if the tests were designed in such a way that a failed
string match didn't cause the whole test to hang without a useful error message.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8438123 [details] [diff] [review]
Part 1 - Make configurability check in Xray defineProperty match the spec. v1

r=me

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8438127 [details] [diff] [review]
Part 3 - Fix up SpecialPowers for Array Xrays. v1

>So we waive [[Object]] instances before
>+  //    inspecting properties.

[[Object]] and [[Array]].

r=me

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8438128 [details] [diff] [review]
Part 4 - Implement Xrays to Arrays. v1

>                 "We should use XrayWrappers for standard ES Object instances "

And Array.

r=me

Comment 10

[Bobby Holley (:bholley)]

All-platform try push: https://tbpl.mozilla.org/?tree=Try&rev=f710f0a08a5b

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 5 - Fix up webconsole. v2

Comment 12

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=af8ee6ebd6bf

Comment 13

[Eric Faust [:efaust]]

Comment on attachment 8438126 [details] [diff] [review]
Part 2 - Inherit property descriptor attributes for pre-existing own value props in Proxy::set. v1

Review of attachment 8438126 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, I guess. This is correct, but this whole hasPrototype business causes us to "redo" so much logic from the property access path in jsproxy.cpp. It has always, and continues to seem fragile because of patches like this. grumble.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8438598 [details] [diff] [review]
Part 5 - Fix up webconsole. v2

Review of attachment 8438598 [details] [diff] [review]:
-----------------------------------------------------------------

Some of these additions won't work when the actor is running in a worker (no Components), but that was already the case before this patch, so let's just ignore it for now.

::: toolkit/devtools/server/actors/script.js
@@ +3620,5 @@
>  
>      let raw = obj.unsafeDereference();
>      let items = aGrip.preview.items = [];
>  
> +    for (let i = 0; i < raw.length; ++i) {

Since accessing raw.length might be unsafe and we have already calculated the length in the beginning of this function, might as well use that instead.

@@ +3704,5 @@
>      let entries = aGrip.preview.entries = [];
> +    // We don't have Xrays to Iterators, so .entries returns [key, value]
> +    // Arrays that live in content. But since we have Array Xrays,
> +    // we'll deny access depending on the nature of those values. So we need
> +    // to waive Xrays on those tupes (and re-apply them on the underlying

Typo: types.

::: toolkit/devtools/webconsole/utils.js
@@ +1752,2 @@
>     *
> +   * @param val aValue

Nit: @param any aValue

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 6 - Fix up mozApps for Array Xrays. v1

Comment 16

[Bobby Holley (:bholley)]

Thanks for the review past!

I had to correct one webconsole test to account for the fact that print() now prints more useful stuff. Doing an addendum try push to the one in comment 12:

https://tbpl.mozilla.org/?tree=Try&rev=7682b12de4c6

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 8438687 [details] [diff] [review]
Part 6 - Fix up mozApps for Array Xrays. v1

Review of attachment 8438687 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable to me...

Comment 18

[Bobby Holley (:bholley)]

Collectively-green try push:

https://tbpl.mozilla.org/?tree=Try&rev=f710f0a08a5b
https://tbpl.mozilla.org/?tree=Try&rev=af8ee6ebd6bf
https://tbpl.mozilla.org/?tree=Try&rev=7682b12de4c6

Landed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a03d78cf4635
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/414392c43876
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d05d0e256055
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf161d62981
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/351313b35331
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/17b0811fc6ae

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a03d78cf4635
https://hg.mozilla.org/mozilla-central/rev/414392c43876
https://hg.mozilla.org/mozilla-central/rev/d05d0e256055
https://hg.mozilla.org/mozilla-central/rev/aaf161d62981
https://hg.mozilla.org/mozilla-central/rev/351313b35331
https://hg.mozilla.org/mozilla-central/rev/17b0811fc6ae

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8438687 [details] [diff] [review]
Part 6 - Fix up mozApps for Array Xrays. v1

This approval request covers the patches that landed in this bug, as well as those from bug 1022016 (which already has an aurora approval request for other reasons) and bug 1020460.

The basic issue is that we want to ship this and Object Xrays (bug 987111) together. I've discussed this offline with the release drivers (thread title: "Aurora Uplift for Array Xrays"), and gotten the OK.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8438687 [details] [diff] [review]
Part 6 - Fix up mozApps for Array Xrays. v1

I spoke with Bobby about this offline. The plan is to land and monitor for add-on breakage, which we don't really expect to see until beta. Bobby tells me that he should be able to disable this change if there is significant add-on breakage (either many add-ons or high profile add-ons).

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/43fe37d13da6
https://hg.mozilla.org/releases/mozilla-aurora/rev/e1f792277665
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf0dad67e829
https://hg.mozilla.org/releases/mozilla-aurora/rev/e3022e313a05
https://hg.mozilla.org/releases/mozilla-aurora/rev/d3a95ceb20df
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e56e197cdbf

Comment 23

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/164b61458ca5

Comment 24

[Will Bamberg [:wbamberg]]

I'm marking this as dev-doc-complete on the understanding that https://developer.mozilla.org/en-US/docs/Xray_vision, and especially https://developer.mozilla.org/en-US/docs/Xray_vision#Xray_semantics_for_Object_and_Array, covers it. If it doesn't, please let me know.