Closed
Bug 839792
Opened 12 years ago
Closed 12 years ago
From add-on, TypeError: "_appendCurrentResult" is read-only
Categories
(Core :: XBL, defect)
Core
XBL
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: Mardak, Assigned: bholley)
References
Details
(Keywords: regression)
Attachments
(3 files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Someone filed a bug for Enter Selects not working on Feb 9 Nightly, and I looked into it and found an exception when the add-on tries to insert itself into the normal display flow of autocomplete results:
TypeError: "_appendCurrentResult" is read-only
The add-on tries to replace the existing xbl method with a function that calls the original method and adds some additional functionality.
I'm guessing bug 821850 might be related as those changesets landed Feb 8 and mention xbl and security.
Comment 1•12 years ago
|
||
Bobby, this sounds like fallout from making fields accessor props, right? I recall you had a defineProperty for that for a browser usage, but it wasn't clear to me why that was needed at all... Why was it needed?
Reporter | ||
Comment 2•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #1)
> I recall you had a defineProperty for that for a browser usage
Things seem to work if I do:
Object.defineProperty(obj, prop, {
configurable: true,
value: val,
writable: true
})
.. instead of:
obj[prop] = val;
Looking through the changesets, there was this added comment:
+ // onSearchComplete is implemented as an XBL method, which is readonly on
+ // the prototype. This means that setting it on the derived/bound object
+ // is an error in strict mode if there's no |own| property. Use defineProperty
+ // to explicitly make it so.
http://hg.mozilla.org/mozilla-central/rev/5264dc88e9c25eba22675f2cd2b8511e941fe730
Comment 3•12 years ago
|
||
Bobby, I wonder... We did the method non-configurable on the proto so that Xrays can use it, right? Do they have to work with the page-visible proto? If so, how bad is it to also define all methods as writable value props on the instances? Seems kinda hacky....
I really wish we could do something where the user can shadow easily via sets but can't change the value on the proto.
Updated•12 years ago
|
Component: Autocomplete → XBL
Product: Toolkit → Core
Comment 5•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #3)
> I really wish we could do something where the user can shadow easily via
> sets but can't change the value on the proto.
Sounds like writable-replaceable to me, i.e. a getter/setter where the setter creates a shadowing value. If I'm understanding right. Which is not necessarily a given.
Comment 6•12 years ago
|
||
That's a huge annoyance, sadly: the value is a JSFunction, so returning it via a getter is a PITA in terms of both rooting and performance. :(
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #1)
> Bobby, this sounds like fallout from making fields accessor props, right?
It's not for field accessors, it's for everything on the Xray prototype. In this case, we're dealing with a method.
> I recall you had a defineProperty for that for a browser usage, but it wasn't
> clear to me why that was needed at all... Why was it needed?
Because that it causes the operation to be a defineProperty on the base object, rather than a set with the proto as the receiver. The latter works, the former does not.
(In reply to Boris Zbarsky (:bz) from comment #3)
> Bobby, I wonder... We did the method non-configurable on the proto so that
> Xrays can use it, right? Do they have to work with the page-visible proto?
Not necessarily, no. You and I just decided on IRC that it was the most efficient path to victory. We could also store them some other way, in the XBL scope if we wanted.
> If so, how bad is it to also define all methods as writable value props on
> the instances? Seems kinda hacky....
Yeah, I'm not really wild about that idea.
> Sounds like writable-replaceable to me, i.e. a getter/setter where the
> setter creates a shadowing value. If I'm understanding right. Which is not
> necessarily a given.
> That's a huge annoyance, sadly: the value is a JSFunction, so returning it
> via a getter is a PITA in terms of both rooting and performance. :(
Couldn't we just use JSPropertyOps and make it a value setter? The getter would just come out of the slot, and the setter would redirect to the correct receiver, if we can figure that out given the signature. Do you guys think that would work?
Comment 8•12 years ago
|
||
> Couldn't we just use JSPropertyOps
I thought we were trying to kill those off...
In any case, I'm not sure how you'd get the right receiver from a JSPropertyOp unless the property was already on the correct object.
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #8)
> > Couldn't we just use JSPropertyOps
>
> I thought we were trying to kill those off...
Well, the same hold for XBL. The two might have similar lifespans. ;-)
> In any case, I'm not sure how you'd get the right receiver from a
> JSPropertyOp unless the property was already on the correct object.
I'm pretty sure that |obj| in the JSPropertyOp signature is the receiver.
However, I've realized that the bigger problem is that the value setter will never be called if the prop is readonly.
Comment 10•12 years ago
|
||
Yeah, we're trying to kill JSPropertyOp, slowly.
The object that gets passed there is a bit dodgy as to exactly what it is. I don't believe it's the receiver, but I'm not 100% certain.
Assignee | ||
Comment 11•12 years ago
|
||
Waldo, do you have any other ideas on how to solve this problem? If nothing comes to mind, I can also just add an auxiliary object to track this stuff in the XBL compartment. It wastes some memory and adds complexity, but it's not the end of the world.
Updated•12 years ago
|
status-firefox21:
--- → affected
Comment 12•12 years ago
|
||
Ideally you'd use a reserved slot on the function object, or on the object where it lives, or something like that. Not sure which better matches your use case, just reading here.
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> Ideally you'd use a reserved slot on the function object, or on the object
> where it lives, or something like that. Not sure which better matches your
> use case, just reading here.
Well, the problem is that we want to keep track of all of the methods on the "original" XBL prototype. This requires some kind of dictionary that content can't mess with. The hope was to make the content-accessible prototype serve double-purpose in this regard, by freeze/sealing it and making it a permanent prop on the window. But now we're running into this method issue.
At the point when we're talking about adding a whole shadow dictionary, we may as well put that dictionary in the XBL scope and be done with it. I was mostly just curious if you could think of any clever way to have sets just shadow rather than throwing on account of the inherited readonly property. If nothing comes to mind, I'll just mirror the dictionary in the XBL scope.
Comment 14•12 years ago
|
||
Yeah, there's nothing clever that can be done. Strict mode throwing determinations are made by the language, not by the property (JSStrictPropertyOp notwithstanding -- I'm working on getting rid of the strict argument there, slowly).
Assignee | ||
Comment 15•12 years ago
|
||
Ok, I've written up some patches to generate shadow prototypes in the XBL scope. Seem to work locally, pushing to try:
https://tbpl.mozilla.org/?tree=Try&rev=83c8dbfa7b42
Assignee | ||
Comment 16•12 years ago
|
||
Green. Uploading patches and flagging bz for review.
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #712931 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #712933 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 19•12 years ago
|
||
This reverts bug 821850 part 16, and updates the tests accordingly.
Attachment #712935 -
Flags: review?(bzbarsky)
Comment 20•12 years ago
|
||
Comment on attachment 712931 [details] [diff] [review]
Part 1 - Store members on a shadow proto in the XBL scope. v1
r=me
Attachment #712931 -
Flags: review?(bzbarsky) → review+
Comment 21•12 years ago
|
||
Comment on attachment 712933 [details] [diff] [review]
Part 2 - Do XBL lookups on the shadow prototype. v1
Can chrome not create Xrays to other chrome objects?
Or do we just not care about those cases?
r=me
Attachment #712933 -
Flags: review?(bzbarsky) → review+
Comment 22•12 years ago
|
||
Comment on attachment 712935 [details] [diff] [review]
Part 3 - Revert Tamper-proofing. v1
r=me
Would be nice if we had a way to test that the xray bit still works right, yes? Followup bug for that test is ok.
Attachment #712935 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 23•12 years ago
|
||
Sorted out the review comments on IRC. Including here for posterity.
(In reply to Boris Zbarsky (:bz) from comment #21)
> Can chrome not create Xrays to other chrome objects?
Xray only calls into LookupMember when it determines that it's running in an XBL scope.
(In reply to Boris Zbarsky (:bz) from comment #22)
> Would be nice if we had a way to test that the xray bit still works right,
> yes?
We already have such tests. :-)
Assignee | ||
Comment 24•12 years ago
|
||
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/957aa28ca89d
https://hg.mozilla.org/mozilla-central/rev/ade9020c8506
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment 26•12 years ago
|
||
Updated•12 years ago
|
Assignee: nobody → bobbyholley+bmo
You need to log in
before you can comment on or make changes to this bug.
Description
•