1090592 - Investigate CPOWs and SpecialPower wrappers

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

In trying to test bug 1056018, I created a testcase to get document.cookie through a CPOW. However, I used content pages with SpecialPowers for my test and found that we have some sort of bad interaction between CPOWs and the SpecialPowers wrapper. In particular, I *think* this has something to do with Object.getOwnPropertyDescriptor not being completely transparent for CPOWs (either that or the prototype chain not being correct for DOM objects exposed through CPOWs). Basically, I'm very confused and decided to punt on figuring out exactly what's going on for the moment. I'll attach a patch with my testcase in a second.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch that adds a non-working test.

Comment 2

[Gabor Krizsanits (INACTIVE)]

Attachment: cpow proto v1

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #0)
> CPOWs (either that or the prototype chain not being correct for DOM objects

Actually the prototype chain was just broken for CPOWs. Or more like not implemented. SpecialPowers heavily rely on Object.getPrototypeOf which returns null for CPOWs. The CPOWProxyHandler has an mHasPrototype set to false, which means it just shortcuts all the prototype related ops (http://mxr.mozilla.org/mozilla-central/source/js/src/jsproxy.h#164)

So I've changed that and implemented the necessary hook but not sure if we had any reason to not do this before (security or something else). So I need a feedback from Bobby about this just in case, but feel free to steal the feedback from Bobby if you have the answer...

The test does not set the cookie to a=b, but with this patch once I set it now I can receive it, unlike before.

Also there's a lot of things in this patch I figured out how to do, but not 100% sure if I'm doing it right, so a thorough review will be needed, should I flag you or Bill or Bobby for that?

Finally I have no idea how to use TaggedProto::LazyProto in this file, the reinterpret_cast I do in the patch is clearly the wrong way...

I also have performance concerns, since now a simple set operation for a new prop will always do some extra rounds (crawling up on the proto chain), but that's kind of the cost of the correct behavior IMO.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Can we expose the getPrototypeOf hook without setting the hasPrototype hook on the proxy handler? Otherwise the performance cost will be terrible.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(I don't believe that having hasPrototype=false causes correctness problems since the code in the child takes care of walking up the prototype chain. But I could be wrong.)

Comment 5

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #3)
> Can we expose the getPrototypeOf hook without setting the hasPrototype hook
> on the proxy handler? Otherwise the performance cost will be terrible.

We can, yes.

(In reply to Bill McCloskey (:billm) from comment #4)
> (I don't believe that having hasPrototype=false causes correctness problems
> since the code in the child takes care of walking up the prototype chain.
> But I could be wrong.)

Correct.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8559216 [details] [diff] [review]
cpow proto v1

Review of attachment 8559216 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/ipc/WrapperOwner.cpp
@@ +80,5 @@
>  class CPOWProxyHandler : public BaseProxyHandler
>  {
>    public:
>      MOZ_CONSTEXPR CPOWProxyHandler()
> +      : BaseProxyHandler(&family, true) {}

Per what Bill said, I think we should not set HasPrototype=true on the CPOWProxyHandler, since that would cause several unnecessary IPC round-trips. I think it should work fine without it.

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Bill McCloskey (:billm) from comment #4)
> (I don't believe that having hasPrototype=false causes correctness problems
> since the code in the child takes care of walking up the prototype chain.
> But I could be wrong.)

Oh, indeed, I've got that flag all wrong, I thought it's also necessary for the proto tagging (TaggedProto::LazyProto) to take effect which is needed for getting the hook triggered.

Comment 8

[Bobby Holley (:bholley)]

You should just be able to pass Proxy::LazyProto to NewProxyObject, right? If there's not a way to do that right now, we should make one. The issue of a dynamically-reported prototype is conceptually orthogonal to the HasPrototype machinery.

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #8)
> You should just be able to pass Proxy::LazyProto to NewProxyObject, right?
> If there's not a way to do that right now, we should make one. The issue of
> a dynamically-reported prototype is conceptually orthogonal to the
> HasPrototype machinery.

It's defined in jsinfer.h and I just don't feel comfortable about including that here. Not sure what's the policy nor do I know where are the lib boundaries exactly but these ipc files seems to be staying at jsfriendapi level.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We should just add a call like "NewProxyObjectWithLazyPrototype" to jsproxy.h.

Comment 11

[Bobby Holley (:bholley)]

NewProxyObject takes an options object - just add a boolean to that.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Attachment: LazyProto for ProxyOptions. v1

Should I also add a patch that changes all the consumers of this API and passing in TaggedProto::LazyProto directly currently?

Comment 13

[Gabor Krizsanits (INACTIVE)]

Attachment: GetPrototypeOf hook for CPOWProxyHandler. v2

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8559747 [details] [diff] [review]
LazyProto for ProxyOptions. v1

Review of attachment 8559747 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/proxy/Proxy.cpp
@@ +834,1 @@
>      return ProxyObject::New(cx, handler, priv, TaggedProto(proto_), parent_,

I think it's slightly cleaner to have a temporary at the top of this function of type TaggedProto, so that we're not doing TaggedProto(TaggedProto::LazyProto) (which is ok, I think, but raises eyebrows).

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8559749 [details] [diff] [review]
GetPrototypeOf hook for CPOWProxyHandler. v2

Review of attachment 8559749 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Gabor. I'd just like to see this again to make sure the ObjectOrNullVariant changes work out.

::: dom/base/test/chrome/cpows_child.js
@@ +64,5 @@
>  
>    let for_json = { "n": 3, "a": array, "s": "hello", o: { "x": 10 } };
>  
> +  let proto = { data: 42 };
> +  let with_proto = Object.create(proto);

Thanks for the test! Could you please add a test for a null prototype as well (Object.create(null)).

::: js/ipc/PJavaScript.ipdl
@@ +39,5 @@
>      prio(high) sync CallOrConstruct(uint64_t objId, JSParam[] argv, bool construct) returns (ReturnStatus rs, JSVariant result, JSParam[] outparams);
>      prio(high) sync HasInstance(uint64_t objId, JSVariant v) returns (ReturnStatus rs, bool has);
>      prio(high) sync ObjectClassIs(uint64_t objId, uint32_t classValue) returns (bool result);
>      prio(high) sync ClassName(uint64_t objId) returns (nsString name);
> +    prio(high) sync GetPrototypeOf(uint64_t objId) returns (ReturnStatus rs, JSVariant result);

I think it would make more sense if |result| was returned as ObjectOrNullVariant.

::: js/ipc/WrapperAnswer.cpp
@@ +576,5 @@
> +
> +    if (proto) {
> +        JS::RootedValue val(cx, ObjectValue(*proto));
> +
> +        if (!toVariant(cx, val, result))

You can use toObjectOrNullVariant instead and skip the |if (proto)| test.

@@ +580,5 @@
> +        if (!toVariant(cx, val, result))
> +            return fail(cx, rs);
> +    }
> +
> +    LOG("getProrotypeOf(%s) = %s", ReceiverObj(objId), OutVariant(*result));

Typo in getPrototypeOf. We don't have a logging function for ObjectOrNullVariant, but I don't really care if we log this result. It shouldn't be hard to add the logging in JavaScriptLogging.h if you want though.

::: js/ipc/WrapperOwner.cpp
@@ +707,5 @@
> +    if (vp.isUndefined())
> +        return true;
> +
> +    if (!vp.isObject())
> +        return false;

This will return false if the prototype is null, which is a valid prototype I think. So we shouldn't do that.

Comment 16

[Gabor Krizsanits (INACTIVE)]

(In reply to Bill McCloskey (:billm) from comment #15)
> > +    if (vp.isUndefined())
> > +        return true;
> > +
> > +    if (!vp.isObject())
> > +        return false;
> 
> This will return false if the prototype is null, which is a valid prototype
> I think. So we shouldn't do that.

I totally should have added a comment about this. I would have expected the same behavior, but
in practice for null protos we get undefined here. In fact undefined is not a valid prototype
I think. At least you cannot call Object.create with undefined, only with null or an object.
I haven't double checked the spec on this...
So I'm going to investigate why do we get undefined here, and see if the ObjectOrNullVariant
version works out. And thanks for the quick review!

Comment 17

[Gabor Krizsanits (INACTIVE)]

Attachment: GetPrototypeOf hook for CPOWProxyHandler. v3

(In reply to Bill McCloskey (:billm) from comment #15)
> I think it would make more sense if |result| was returned as
> ObjectOrNullVariant.

This really made the code a lot simpler.

> Typo in getPrototypeOf. We don't have a logging function for
> ObjectOrNullVariant, but I don't really care if we log this result. It
> shouldn't be hard to add the logging in JavaScriptLogging.h if you want
> though.

I tried, but it turned out to be harder than it's worth. The object case
is not easy. I either have to pass in a cx to the logging function which
I don't want to, or add a conditional toVariant and temp JSVariant in the function itself which I would also avoid unless we have a good reason for it.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16)
> So I'm going to investigate why do we get undefined here, and see if the
> ObjectOrNullVariant

With the ObjectOrNullVariant version this is a non-issue any longer so I
won't waste time on this for now.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8560442 [details] [diff] [review]
GetPrototypeOf hook for CPOWProxyHandler. v3

Review of attachment 8560442 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again.

::: js/ipc/JavaScriptBase.h
@@ +98,5 @@
>      }
>      bool RecvClassName(const uint64_t &objId, nsString *result) {
>          return Answer::RecvClassName(ObjectId::deserialize(objId), result);
>      }
> +    bool RecvGetPrototypeOf(const uint64_t &objId, ReturnStatus *rs, ObjectOrNullVariant*result) {

Looks like you're missing a space after ObjectOrNullVariant.

::: js/ipc/WrapperAnswer.cpp
@@ +574,5 @@
> +    if (!JS_GetPrototype(cx, obj, &proto))
> +        return fail(cx, rs);
> +
> +    if (!toObjectOrNullVariant(cx, proto, result))
> +        return false;

This should be |return fail(cx, rs);|. If you return false, the IPC code will cause the process to crash. fail(cx, rs) will return true, but it will store information about the exception in |result|.

::: js/ipc/WrapperOwner.cpp
@@ +685,5 @@
> +    FORWARD(getPrototypeOf, (cx, proxy, op));
> +}
> +
> +bool
> +WrapperOwner::getPrototypeOf(JSContext *cx, HandleObject proxy, MutableHandleObject op)

Maybe call it objp instead of op?

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #14)
> Comment on attachment 8559747 [details] [diff] [review]
> LazyProto for ProxyOptions. v1
> 
> Review of attachment 8559747 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/proxy/Proxy.cpp
> @@ +834,1 @@
> >      return ProxyObject::New(cx, handler, priv, TaggedProto(proto_), parent_,
> 
> I think it's slightly cleaner to have a temporary at the top of this
> function of type TaggedProto, so that we're not doing
> TaggedProto(TaggedProto::LazyProto) (which is ok, I think, but raises
> eyebrows).

I was just about to quickly fix this up but I realized that I don't know what you mean exactly.
TaggedProto::LazyProto is actually a JSObject* and not a TaggedProto, so at some point in the code
I do have to convert it to a TaggedProto. This was the least ugly way of doing it, maybe I could add another getter to TaggedProto that does this conversion internally... or let me know if I'm missing something and got your suggestion wrong.

Comment 20

[Bobby Holley (:bholley)]

Ok, that's fine then. Though really LazyProto should be of type TaggedProto - if you can easily make that switch please do, otherwise don't worry about it.

Comment 21

[Gabor Krizsanits (INACTIVE)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba3829bbe60a

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f083c0433964
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/658615540937

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f083c0433964
https://hg.mozilla.org/mozilla-central/rev/658615540937