Closed Bug 1020609 Opened 5 years ago Closed 5 years ago

Implement Xrays to Array objects


(Core :: XPConnect, defect)

Not set



Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed


(Reporter: bholley, Assigned: bholley)


(Blocks 1 open bug)


(Keywords: dev-doc-complete)


(6 files, 1 obsolete file)

This ended up being pretty orthogonal from the TypedArray work, so I decided to split it out from bug 987163.
Depends on: 1020460, 972987, 1015791
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.
Attachment #8438123 - Flags: review?(bzbarsky)
This just makes the special Object behavior apply to Array as well.
Attachment #8438127 - Flags: review?(bzbarsky)
Attachment #8438128 - Flags: review?(bzbarsky)
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

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.
Attachment #8438129 - Flags: review?(past)
Comment on attachment 8438123 [details] [diff] [review]
Part 1 - Make configurability check in Xray defineProperty match the spec. v1

Attachment #8438123 - Flags: review?(bzbarsky) → review+
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]].

Attachment #8438127 - Flags: review?(bzbarsky) → review+
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.

Attachment #8438128 - Flags: review?(bzbarsky) → review+
Attachment #8438129 - Attachment is obsolete: true
Attachment #8438129 - Flags: review?(past)
Attachment #8438598 - Flags: review?(past)
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.
Attachment #8438126 - Flags: review?(efaustbmo) → review+
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
Attachment #8438598 - Flags: review?(past) → review+
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:
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...
Attachment #8438687 - Flags: review?(bzbarsky) → review+
Depends on: 1022016
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.
Attachment #8438687 - Flags: approval-mozilla-aurora?
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).
Attachment #8438687 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1036790
Blocks: 856067
I'm marking this as dev-doc-complete on the understanding that, and especially, covers it. If it doesn't, please let me know.
You need to log in before you can comment on or make changes to this bug.