From add-on, TypeError: "_appendCurrentResult" is read-only

RESOLVED FIXED in Firefox 21

Status

()

Core
XBL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Mardak, Assigned: bholley)

Tracking

({regression})

Trunk
mozilla21
regression
Points:
---

Firefox Tracking Flags

(firefox21+ fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
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

5 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?
Blocks: 821850
tracking-firefox21: --- → ?
Keywords: regression
(Reporter)

Comment 2

5 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

5 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

5 years ago
Component: Autocomplete → XBL
Product: Toolkit → Core

Updated

5 years ago
Duplicate of this bug: 839795
(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

5 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.  :(
(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

5 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.
(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.
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.
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

5 years ago
status-firefox21: --- → affected
tracking-firefox21: ? → +
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.
(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.
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).
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
Green. Uploading patches and flagging bz for review.
Created attachment 712931 [details] [diff] [review]
Part 1 - Store members on a shadow proto in the XBL scope. v1
Attachment #712931 - Flags: review?(bzbarsky)
Created attachment 712933 [details] [diff] [review]
Part 2 - Do XBL lookups on the shadow prototype. v1
Attachment #712933 - Flags: review?(bzbarsky)
Created attachment 712935 [details] [diff] [review]
Part 3 - Revert Tamper-proofing. v1

This reverts bug 821850 part 16, and updates the tests accordingly.
Attachment #712935 - Flags: review?(bzbarsky)

Comment 20

5 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

5 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

5 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+
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. :-)
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/957aa28ca89d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ade9020c8506
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/872af2305af3
https://hg.mozilla.org/mozilla-central/rev/957aa28ca89d
https://hg.mozilla.org/mozilla-central/rev/ade9020c8506
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox21: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/872af2305af3
Assignee: nobody → bobbyholley+bmo
Duplicate of this bug: 839873
You need to log in before you can comment on or make changes to this bug.