CPOWs ignore Xrays waivers

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10s+)

Details

Attachments

(5 attachments)

Assignee

Description

5 years ago
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.
tracking-e10s: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Assignee

Comment 2

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

Comment 4

5 years ago
When we starting forwarding isCallable and isConstructor over CPOWs, calls that
could potentially invoke this proxy trap can now GC.
Attachment #8489999 - Flags: review?(wmccloskey)
Assignee

Comment 7

5 years ago
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.
Attachment #8490003 - Flags: review?(wmccloskey)
Assignee

Comment 8

5 years ago
Attachment #8490004 - Flags: review?(wmccloskey)
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"
Attachment #8490002 - Flags: feedback?(efaustbmo) → feedback+
Assignee

Comment 10

5 years ago
(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 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.
Attachment #8490000 - Flags: review?(wmccloskey) → review+
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.
Attachment #8490003 - Flags: review?(wmccloskey) → review+
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.
Attachment #8490004 - Flags: review?(wmccloskey) → review+
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.
Flags: needinfo?(bobbyholley)
Assignee

Comment 15

5 years ago
(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.
Flags: needinfo?(bobbyholley)
Attachment #8489999 - Flags: review?(wmccloskey) → review+
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.
Attachment #8490002 - Flags: review?(wmccloskey) → review+

Updated

5 years ago
Depends on: 1075543

Updated

4 years ago
Duplicate of this bug: 865399
You need to log in before you can comment on or make changes to this bug.