Closed
Bug 1020609
Opened 10 years ago
Closed 10 years ago
Implement Xrays to Array objects
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bholley, Assigned: bholley)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(6 files, 1 obsolete file)
2.27 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.56 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
3.63 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
16.33 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
5.43 KB,
patch
|
past
:
review+
|
Details | Diff | Splinter Review |
1.38 KB,
patch
|
mccr8
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This ended up being pretty orthogonal from the TypedArray work, so I decided to split it out from bug 987163.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=12e2c896f82a
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8438126 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 4•10 years ago
|
||
This just makes the special Object behavior apply to Array as well.
Attachment #8438127 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8438128 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 6•10 years ago
|
||
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.
Attachment #8438129 -
Flags: review?(past)
Comment 7•10 years ago
|
||
Comment on attachment 8438123 [details] [diff] [review] Part 1 - Make configurability check in Xray defineProperty match the spec. v1 r=me
Attachment #8438123 -
Flags: review?(bzbarsky) → review+
Comment 8•10 years ago
|
||
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
Attachment #8438127 -
Flags: review?(bzbarsky) → review+
Comment 9•10 years ago
|
||
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
Attachment #8438128 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 10•10 years ago
|
||
All-platform try push: https://tbpl.mozilla.org/?tree=Try&rev=f710f0a08a5b
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8438129 -
Attachment is obsolete: true
Attachment #8438129 -
Flags: review?(past)
Attachment #8438598 -
Flags: review?(past)
Assignee | ||
Comment 12•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=af8ee6ebd6bf
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8438687 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 16•10 years ago
|
||
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•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
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
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Assignee | ||
Comment 20•10 years ago
|
||
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 21•10 years ago
|
||
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+
Comment 22•10 years ago
|
||
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
Assignee | ||
Comment 23•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/164b61458ca5
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 24•10 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•