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

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Ed Lee :Mardak]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Ed Lee :Mardak]

(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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Jeff Walden [:Waldo]]

(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

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Bobby Holley (:bholley)]

(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

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Bobby Holley (:bholley)]

(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

[Jeff Walden [:Waldo]]

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.

Comment 11

[Bobby Holley (:bholley)]

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.

Comment 12

[Jeff Walden [:Waldo]]

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.

Comment 13

[Bobby Holley (:bholley)]

(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

[Jeff Walden [:Waldo]]

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).

Comment 15

[Bobby Holley (:bholley)]

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

Comment 16

[Bobby Holley (:bholley)]

Green. Uploading patches and flagging bz for review.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 1 - Store members on a shadow proto in the XBL scope. v1

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 2 - Do XBL lookups on the shadow prototype. v1

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 3 - Revert Tamper-proofing. v1

This reverts bug 821850 part 16, and updates the tests accordingly.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 712931 [details] [diff] [review]
Part 1 - Store members on a shadow proto in the XBL scope. v1

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Bobby Holley (:bholley)]

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. :-)

Comment 24

[Bobby Holley (:bholley)]

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

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/957aa28ca89d
https://hg.mozilla.org/mozilla-central/rev/ade9020c8506

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/872af2305af3