1065811 - CPOWs ignore Xrays waivers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

I rediscovered this while writing test coverage for bug 1052096 (and have included todo() test coverage for this issue in that bug).

The basic problem is that we strip all wrappers off CPOW targets before putting them in the map. This is necessary because parent->child edges are weak, and so pointing to a CCW in the child would mean that the CPOW would die if the CCW was collected, even if the underlying target was still alive.

This is likely to improve addon-compat. I seem to recall deciding with Bill a while back to explicitly not support this (since it would increase the chances of running content script). But we just discussed it again and decided it's worth doing.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=b86df33482e1

Comment 2

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #1)
> https://tbpl.mozilla.org/?tree=Try&rev=b86df33482e1

All green except for some rooting hazards:

https://tbpl.mozilla.org/?tree=Try&rev=beca49ed200f

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=9437eff07744

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix some soon-to-be rooting hazards. v1

When we starting forwarding isCallable and isConstructor over CPOWs, calls that
could potentially invoke this proxy trap can now GC.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Expose isCallable()/isConstructor() in JS_PUBLIC_API. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Stop statically computing callability/constructibility on CPOWs. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Clean up ObjectId handling with static type checking. v1

While adding the CPOW flag for xray waivers, I discovered a bunch of
inconsistency and sloppiness with respect to our handling of object ids,
and a general lack of clarity about when the id included flags or not. Given
the fact that I'm removing static callability for CPOWs, we _could_ just get
rid of the flags, and store the xray waiver state on the answer-side only. But
I eventually decided that these kinds of flags (which are accessible to both
the Answer _and_ the Owner) had enough potential utility that they were worth
cleaning up.

It's worth noting that that utility comes with the large caveat that the flags
can't be trusted for security-sensitive decisions (at least in the parent->child
case), since they could be forged by a compromised child.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Track Xray waivers with CPOWs. v1

Comment 9

[Eric Faust [:efaust]]

Comment on attachment 8490002 [details] [diff] [review]
Part 3 - Stop statically computing callability/constructibility on CPOWs. v1

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

Looks fine to me.

::: js/ipc/WrapperOwner.cpp
@@ +670,5 @@
> +
> +bool
> +CPOWProxyHandler::isConstructor(JSObject *obj) const
> +{
> +    return OwnerOf(obj)->isConstructor(obj);

Do we need to do something about if the IPC link is broken here and in CPOWProxyHandler::isCallable? If so, I assume we should just say "false"

Comment 10

[Bobby Holley (:bholley)]

(In reply to Eric Faust [:efaust] from comment #9)
> Do we need to do something about if the IPC link is broken here and in
> CPOWProxyHandler::isCallable? If so, I assume we should just say "false"

That happens in both WrapperOwner::isCallable in the parent (if the link to the child is severed) and WrapperAnswer::AnswerIsCallable (if the CPOW has been GCed). I think we've got our bases covered.

Comment 11

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

Comment on attachment 8490000 [details] [diff] [review]
Part 2 - Expose isCallable()/isConstructor() in JS_PUBLIC_API. v1

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

::: js/src/jsapi.cpp
@@ +3982,5 @@
>  
> +namespace JS {
> +
> +JS_PUBLIC_API(bool)
> +IsCallable(JSObject *obj)

We already have JS_ObjectIsCallable. Did you want to get rid of the cx argument?

I know it's a pain, but I think it would be better to make JS_ObjectIsCallable work the way you want. I see about 20 callees, so it wouldn't be too horrible to fix them all.

Comment 12

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

Comment on attachment 8490003 [details] [diff] [review]
Part 4 - Clean up ObjectId handling with static type checking. v1

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

Thanks for fixing this!

::: js/ipc/JavaScriptShared.h
@@ +18,5 @@
>  
> +class ObjectId {
> +  public:
> +    // Use 47 at most, to be safe, since jsval privates are encoded as doubles.
> +    // XXXbholley - What is special about 47, exactly?

I think the reasoning here was kinda bogus. The only thing we need to guarantee is that casting our serial number to a Value and then calling isDouble() on it will yield true.

Based on my reading of Value.h, this means that all our serial numbers must be less than 0x1fff0000ffffffff on 32-bit and 0xfff80000ffffffff on 64-bit. Those are a lot bigger than 1<<47, so I guess we're being conservative.

Can you just comment to this effect here?

@@ +24,5 @@
> +    static const size_t FLAG_BITS = 1;
> +    static const uint64_t SERIAL_NUMBER_MAX = (uint64_t(1) << SERIAL_NUMBER_BITS) - 1;
> +
> +    explicit ObjectId(uint64_t serialNumber, bool isCallable)
> +      : serialNumber_(serialNumber) , isCallable_(isCallable)

No space before the comma.

Comment 13

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

Comment on attachment 8490004 [details] [diff] [review]
Part 5 - Track Xray waivers with CPOWs. v1

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

Thanks. This looks nice.

::: js/ipc/JavaScriptLogging.h
@@ +95,5 @@
>          const char *side, *objDesc;
>  
>          if (local == incoming) {
>              JS::RootedObject obj(cx);
> +            obj = shared->findObjectById(cx, id);

This doesn't seem right to me. This function throws an exception if the object is dead, and the exception won't be reported here. I'd rather stick with what we have if we can. If not, I guess we can add an extra test here to check for dead objects first.

Comment 14

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

Bobby, can you explain the change for IsCallable? It seems better and simpler to me to do that stuff statically. Is it possible for an object to change its callability dynamically? Or did you just want to steal that bit for xrays? I think we have plenty of bits, so the latter doesn't seem like a good reason to me.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #14)
> Bobby, can you explain the change for IsCallable?

Efaust's recent refactorings have made isCallable and isConstructor bonafide proxy traps, and moved us a way from the old world where we had two separate JSClasses for callable and non-callable. That was the only reason we had to track this information statically.

> It seems better and simpler to me to do that stuff statically.

Why? As a cache for performance? There might be an argument to be made there, but if that's the case then it's very arbitrary to cache the answer of isCallable but not the answer of other things proxy traps that are unlikely to change like className or objectClassIs. I don't think we should prematurely optimize here.

> Is it possible for an object to change its callability dynamically?

In theory, yes.

> Or did you just want to steal that bit for xrays?

This was the original thing that caused me to notice this (before I refactored all of the ObjectId stuff), but as you note it's not a reason to go forward with the change. The reason is that this removes complexity from the CPOW layer and removes a mechanism that doesn't make sense with the current proxy architecture.

Comment 16

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

Comment on attachment 8490002 [details] [diff] [review]
Part 3 - Stop statically computing callability/constructibility on CPOWs. v1

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

OK, I sort of agree. This feels like more code than before, but I guess it's just because there's so much boilerplate everywhere.

::: js/ipc/WrapperOwner.cpp
@@ +650,5 @@
>  
>  bool
>  CPOWProxyHandler::isCallable(JSObject *obj) const
>  {
>      return OwnerOf(obj)->isCallable(obj);

Most of the hooks call FORWARD, which automatically checks if active() is true on the WrapperOwner. If it's false, we're not allowed to send an IPDL message, so we have to return a dummy value. We can't use FORWARD because we don't have a cx, but we still need to check active(). See CPOWProxyHandler::className as an example.

@@ +670,5 @@
> +
> +bool
> +CPOWProxyHandler::isConstructor(JSObject *obj) const
> +{
> +    return OwnerOf(obj)->isConstructor(obj);

Same as above.

Comment 17

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=0d53781ad606

Comment 18

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=0c414674031f

Comment 19

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c477d2a7ab65
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d9edbc602839
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8fb1ab37e3e0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bc71142337a3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4d94f2bf456a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c477d2a7ab65
https://hg.mozilla.org/mozilla-central/rev/d9edbc602839
https://hg.mozilla.org/mozilla-central/rev/8fb1ab37e3e0
https://hg.mozilla.org/mozilla-central/rev/bc71142337a3
https://hg.mozilla.org/mozilla-central/rev/4d94f2bf456a