965094 - Reconcile the JSObject* argument behavior for Func on interfaces and on interface members

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

For interface members, we call the Func with the JSContext in the caller compartment but the JSObject the global of actual object that we're defining the property on.  This matter in the Xray case, when the JSObject will be the global of the actual DOM object the Xray wraps, while the JSContext is in the Xray compartment.

For interfaces we call it with the JSContext in the compartment of obj.  And obj is either the window or the Xray, depending.

I think (and Bobby agrees) what we should do is for the Xray case call it with the JSContext in the Xray compartment but the object in the content compartment.

I'll do that after bug 945416 lands so I don't bitrot it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Reconcile the behavior of the JSObject* argument for Func on interfaces and on interface members.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Conceivably, we should rename ThreadsafeCheckIsChrome to ThreadsafeIsCallerChrome.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8368425 [details] [diff] [review]
Reconcile the behavior of the JSObject* argument for Func on interfaces and on interface members.

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

::: dom/apps/src/InterAppComm.cpp
@@ +11,5 @@
>  
>  using namespace mozilla::dom;
>  
>  /* static */ bool
> +InterAppComm::EnabledForScope(JSContext* aCx, JS::Handle<JSObject*> aObj)

Did we determine that we want these constructors for Xray callers?

::: dom/base/Navigator.cpp
@@ +1963,4 @@
>    nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(global);
>  
>    return win &&
>           CheckPermission(win, "downloads")  &&

In this case, did we determine that we _don't_ want to see this over Xrays?

::: dom/bindings/BindingUtils.cpp
@@ +2144,5 @@
>    return true;
>  }
>  
>  bool
>  ThreadsafeCheckIsChrome(JSContext* aCx, JSObject* aObj)

This behavior change is a bit scary, because we're essentially switching to a weaker check (we never have a non-system caller resolving something on a system object, but we do have the converse). And the comments surrounding some of the current consumers of this API worry me (in particular those in CGConstructorEnabledChromeOnly, which will no longer be accurate after this patch).

I agree that this function is ambiguously-named and should be renamed. So I think we should do it here in this patch, which will give us an excuse to methodically go through those callers, verify that this is the behavior we want, and update the comments.

@@ +2147,5 @@
>  bool
>  ThreadsafeCheckIsChrome(JSContext* aCx, JSObject* aObj)
>  {
>    using mozilla::dom::workers::GetWorkerPrivateFromContext;
> +  if (NS_IsMainThread()){

Nit: Whitespace

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Did we determine that we want these constructors for Xray callers?

I don't know, for InterAppComm.  For that one I'm just preserving existing behavior...

> In this case, did we determine that we _don't_ want to see this over Xrays?

Yes.  See bug 957592 comment 60.  I thought I'd referenced it here, but looks like I forgot to, sorry.

> which will no longer be accurate after this patch

I think that comment predates us having the ability to do usefully different visibility over Xrays and non-Xrays, so we used to not be able to have the behavior the patch is implementing at all.

I'll do the renaming and such.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, for reference I did audit the callers of ThreadsafeCheckIsChrome and I believe they all want the "new" behavior.  I put new in quotes because there are in fact only two callers.  
One is CGConstructorEnabledChromeOnly, and the old behavior there is that obj and cx are in the same compartment (the compartment of cx in the new behavior, so the old check is in fact exactly the same as the new check.  The other caller is Navigator::HasDownloadsSupport, which I'm changing in this patch to not call the method anymore.

Comment 6

[Bobby Holley (:bholley)]

Ok. Also, shouldn't there be a part of this patch that actually changes what we pass in those cases? Or did that already happen in bug 945416? If the latter, we should make sure this lands before the branch or is backported.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Reconcile the behavior of the JSObject* argument for Func on interfaces and on interface members.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: This ended up in the try changeset somehow...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Reconcile the behavior of the JSObject* argument for Func on interfaces and on interface members.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8369679 [details] [diff] [review]
Reconcile the behavior of the JSObject* argument for Func on interfaces and on interface members.

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

r=me with comments addressed, though doing so may involve significantly munging the patch.

::: dom/base/nsDOMClassInfo.cpp
@@ +2868,5 @@
> +          return NS_ERROR_DOM_SECURITY_ERR;
> +        }
> +      } else {
> +        global = obj;
> +      }

I think this should all boil down to:

JS::Rooted<JSObject*> global(cx, js::CheckedUnwrap(obj, /* stopAtOuter = */ false);
if (!global) {
  return NS_ERROR_DOM_SECURITY_ERR;
}

right?

::: dom/bindings/BindingUtils.cpp
@@ +2145,1 @@
>  {

Is there any reason this shouldn't just forward to nsContentUtils::ThreadsafeIsCallerChrome?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Reconcile the behavior of the JSObject* argument for Func on interfaces and on interface members.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff from previous patch

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8369713 [details] [diff] [review]
Reconcile the behavior of the JSObject* argument for Func on interfaces and on interface members.

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

r=bholley with that fixed.

::: dom/base/nsDOMClassInfo.cpp
@@ +2860,5 @@
> +      // actual object we pass in the underlying object in the Xray case.  That
> +      // way the callee can decide whether to allow access based on the caller
> +      // or the window being touched.
> +      JS::Rooted<JSObject*> global(cx,
> +        js::CheckedUnwrap(obj, /* stopAtOuter = */ false));

We need to check and throw here if !global, right?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> We need to check and throw here if !global, right?

Good catch.  Added that back.

Comment 15

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fba6123d70f

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3fba6123d70f