Boris Zbarsky [:bzbarsky] Assignee
	Description • 11 years ago

In preparation for WebIDL Window.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 11 years ago

Bobby, do you have any idea what the comment at http://hg.mozilla.org/mozilla-central/file/1426ffa9caf2/dom/base/nsDOMClassInfo.cpp#l3468 is talking about?  Seems to me like we're entering the compartment of "obj", so it shouldn't matter which JSContext we're using there, right?  Esp. since we rethrow any exceptions on cx anyway!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 11 years ago

Hmm.  That comment was added in bug 158049.  Bug 158049 comment 22 seems to sum up what the problem it was trying to fix was.  I will claim that in today's world this is a nonissue and we can just use cx....

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 11 years ago

And a related question: is this part:

  if (!my_context || !my_context->IsContextInitialized()) {
    // The context is not yet initialized so there's nothing we can do
    // here yet.

    return NS_OK;
  }

actually still relevant?  Should we actually expect to land in NewResolve if my_context is not set up?  And if we do, do we care?

	Bobby Holley (:bholley)
	Comment 4 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #2)
> Hmm.  That comment was added in bug 158049.  Bug 158049 comment 22 seems to
> sum up what the problem it was trying to fix was.  I will claim that in
> today's world this is a nonissue and we can just use cx....

Yes. The idea of a context "having an origin" is a pretty likely indicator that the code is outdated.

Per IRC discussion, I think the resolve hook should still return a value and let the caller do what it wants.

Fixing up JS_ResolveStandardClasses is basically going to be impossible, but that's fine. I've already more or less added support for this over Xrays in bug 933681, and we can just do a final JS_ResolveStandardClasses call for non-Xray outside of the proper Resolve hook (which would have return-and-define-yourself semantics and be shared between Xray and non-Xray).

WebIDL constructors are another interesting case. I don't have a great idea of how that interacts with Xrays in the current world. In particular, an N-1 mapping between objects and the did-we-resolve-this-constructor-on-the-object state doesn't make sense to me. Is that what we do? If so, how does it work? Should we maybe have a separate bank of cached constructors on each XrayHolder for Window?

I think it's pretty important that we move away from the model where we have to pass an XrayHolder into a DOM resolve hook and expect it to deal - doing so is super error-prone.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 11 years ago

> Is that what we do?

No.  If you delete the webidl constructor on the Xray, it'll just come back magically.  We only provide the sane behavior for the actual window, via its proto-and-iface array.  I have no problem with leaving it like that to the extent that it simplifies implementation.

So I was thinking about this.  In the new world where we need to return a value, I think the right logic for webidl ctors as they are today could be something like this:
  
  Maybe<JSAutoCompartment> ac;
  JS::Rooted<JSObject*> global(cx);
  bool haveXray = xpc::WrapperFactory::IsXrayWrapper(obj);
  if (haveXray) {
    // Check if enabled on obj, bail if not  
    global = js::CheckedUnwrap(obj, false);
    if (!global) return false;
    ac.construct(cx, global);
  } else {
    global = obj;
  }
    
  bool defineOnGlobal = checkIfEnabledOnGlobal();
  if (!defineOnGlobal && !defineOnXray) {
    // Just bail
    return true;
  }

  // If !defineOnXray, then we're resolving on the actual global.  In that case,
  // We want to tell the define() call to _not_ define so we can return the value
  // and only do the defining once.
  // If defineOnXray, then we'll return the value to get defined on the xray, but we
  // want define() to define on the global if defineOnGlobal.
  defineOnGlobal = defineOnGlobal && defineOnXray;
  JS::Rooted<JSObject*> interfaceObject(cx, define(cx, global, id, defineOnGlobal));
  if (!interfaceObject) {
    return false;
  }
  rval.setObject(*interfaceObject);
  return true;

What I have no idea how to do is how to make this work with the proposed patches in bug 918567.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 11 years ago

Other remaining questions:

1)  See comment 3.
2)  I don't see how nsIScriptExternalNameSet can work with the "return a value" setup.
    We'll presumably need to special-case that like we do standard classes or something. 
    Except it's way more special...  We _really_ want to handle it the same place we
    handle the rest of the script namespace manager bits.  Or can we kill it off?
3)  nsDOMConstructor::Install installs non-enumerable properties.  Can we just change to
    installing those constructors as enumerable props, since that's what our normal
    resolve code does?

I _think_ the rest should be annoying but straightforward.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 11 years ago

Fwiw, searching addons mxr for "JavaScript-global-dynamic-nameset" gives no hits.  But I have no idea whether binary addons might be using it...

	Bobby Holley (:bholley)
	Comment 8 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #6)
> Other remaining questions:
> 
> 1)  See comment 3.

I think the checks for the initialized context can probably go away.

> 2)  I don't see how nsIScriptExternalNameSet can work with the "return a
> value" setup.
>     We'll presumably need to special-case that like we do standard classes
> or something. 
>     Except it's way more special...  We _really_ want to handle it the same
> place we
>     handle the rest of the script namespace manager bits.  Or can we kill it
> off?

Let's kill it off. :-)

> 3)  nsDOMConstructor::Install installs non-enumerable properties.  Can we
> just change to
>     installing those constructors as enumerable props, since that's what our
> normal
>     resolve code does?

That sounds pretty reasonable.

	Masatoshi Kimura [:emk]
	Comment 9 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #6)
> 3)  nsDOMConstructor::Install installs non-enumerable properties.  Can we
> just change to
>     installing those constructors as enumerable props, since that's what our
> normal
>     resolve code does?
> 
> I _think_ the rest should be annoying but straightforward.

According to the WebIDL spec, DOM constructors must be non-enumerable.
http://heycam.github.io/webidl/#dfn-interface-object

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 11 years ago

Er, right.  And dom::DefineConstructor uses 0 too.

OK, sounds like at the very least we need to return a struct with Value + flags instead of just a Value.  Or two separate outparams.

	Bobby Holley (:bholley)
	Comment 12 • 11 years ago

(Note - this patch is untested, and bz is going to take it from here).

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 11 years ago

Comment on attachment 8361870 [details] [diff] [review]
Do an explicit resolve on underlying WebIDL objects before invoking resolve hooks over Xray. v1

I think we should call mResolveOwnProperty on the unwrapped obj too; otherwise content can observe the resolve.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 11 years ago

Try push at https://tbpl.mozilla.org/?tree=Try&rev=6f1aba16eee3

We could also move the GlobalResolve code into nsGlobalWindow, but that would lose the blame and might need more surgery on it because it uses other classinfo bits; I'd rather not do that here.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 23 • 11 years ago

Comment on attachment 8363403 [details] [diff] [review]
part 3.  Make GlobalResolve fill in a JSPropertyDescriptor that nsWindowSH::NewResolve then uses to actually define the property.

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

::: dom/base/nsDOMClassInfo.cpp
@@ +2986,5 @@
>          }
>  
> +        FillPropertyDescriptor(desc, obj, 0, JS::ObjectValue(*interfaceObject));
> +      } else {
> +        // We've already define the property.  We indicate this to the caller by

defined

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 11 years ago

Update try run: https://tbpl.mozilla.org/?tree=Try&rev=fb2b8da90f1a

	Bobby Holley (:bholley)
	Comment 25 • 11 years ago

Comment on attachment 8363451 [details] [diff] [review]
part 2.  When invoking a DoNewResolve hook on an Xray, make sure to first invoke it on the underlying object.

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

Ugh, this is really complicated. Anyway, r=bholley with comments.

::: dom/base/nsDOMClassInfo.cpp
@@ +2925,5 @@
>        name_struct->mType == nsGlobalNameStruct::eTypeInterface ||
>        name_struct->mType == nsGlobalNameStruct::eTypeClassProto ||
>        name_struct->mType == nsGlobalNameStruct::eTypeClassConstructor) {
>      // Lookup new DOM bindings.
> +    mozilla::dom::DefineInterface define = name_struct->mDefineDOMInterface;

Calling this local 'define' is pretty confusing. I think we should rename it to 'getOrCreateInterfaceObject'.

@@ +2932,5 @@
>            !OldBindingConstructorEnabled(name_struct, aWin, cx)) {
>          return NS_OK;
>        }
>  
> +      if (name_struct->mConstructorEnabled &&

The fact that this is a callback and that it depends on the scope of the caller is non-obvious. How about:

auto checkEnabledForScope = name_struct->mConstructorEnabled;
if (checkEnabledForScope && !checkEnabledForScope(cx, obj)) {
  return NS_OK;
}

@@ +2941,4 @@
>        Maybe<JSAutoCompartment> ac;
>        JS::Rooted<JSObject*> global(cx);
> +      bool isXray = xpc::WrapperFactory::IsXrayWrapper(obj);
> +      if (isXray) {

I think we should probably just write the logic for these two cases separately, given that, after patch 3, they'll be pretty different. But we might as well do it there to avoid churning your patches. I'll comment there.

@@ +3409,5 @@
>      return NS_OK;
>    }
>  
> +  if (isXray) {
> +    // We promise to resolve on the underlying object first.

Explain the subtleties here. In particular about how GlobalResolve will cause the constructors to be stashed in the slots on the global, and how we need to avoid the ambiguity with the 'resolved-then-deleted' case.

@@ +3419,5 @@
> +      // resolve anything on it here.  Otherwise code that expects to see
> +      // cross-origin-accessible properties that XPConnect puts on our
> +      // prototype will get security exceptions when it shouldn't.  XXXbz
> +      // should this have been an UncheckedUnwrap?
> +      return NS_OK;

I think this should be an UncheckedUnwrap, with no null-check. In the case of non-subsuming Xrays, the security policy should have already filtered out the properties that we're not allowed to see. This is just about making an Xray access work right once we've decided to do it.

::: dom/bindings/Codegen.py
@@ +7727,5 @@
>                                           args, getThisObj="",
>                                           callArgs="")
>      def generate_code(self):
>          return CGIndenter(CGGeneric(
> +                "{\n"

Add a comment here that this is happening for Xrays, and explaining that we're performing the resolve on the underlying object first, per-contract.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 11 years ago

> Ugh, this is really complicated.

Yes, yes it is.

> I think we should rename it to 'getOrCreateInterfaceObject'.

Well, but it can define it too...  Peter, thoughts?

> I think this should be an UncheckedUnwrap, with no null-check.

OK, good.

Will fix the comments.

	Bobby Holley (:bholley)
	Comment 27 • 11 years ago

Comment on attachment 8363403 [details] [diff] [review]
part 3.  Make GlobalResolve fill in a JSPropertyDescriptor that nsWindowSH::NewResolve then uses to actually define the property.

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

::: dom/base/nsDOMClassInfo.cpp
@@ +1830,5 @@
> +  nsresult rv = ResolvePrototype(sXPConnect, win, cx, global, mData->mNameUTF16,
> +                                 mData, nullptr, nameSpaceManager, proto,
> +                                 &desc);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (!found && desc.object() && !desc.value().isUndefined() &&

We should rename |found| to |shadowedByContentProperty| or something.

@@ +2737,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  FillPropertyDescriptor(ctorDesc, obj, 0, v);
> +  // And make sure we wrap the value into the right compartment
> +  if (!JS_WrapValue(cx, ctorDesc.value())) {

How about just passing aAllowWrapping = true in the call to WrapNative above, and removing the call to JS_WrapValue?

@@ +2912,1 @@
>  {

This whole setup is really tricky, and I think needs more commenting. I don't know if you have plans to do that at some point, but I just spent a while figuring everything out, and I'd hope never to have to do so again.

So here's a start for a comment, which I'd tentatively put right above the declaration of |defineOnXray|.

// The DOM constructor resolve machinery interacts with Xrays in tricky ways,
// and there are some asymmetries that are important to understand.
//
// In the regular (non-Xray) case, we only want to resolve constructors once (so
// that if they're deleted, they don't reappear). We do this by stashing the
// constructor in a slot on the global, such that we can see during resolve
// whether we've created it already. This is rather memory-intensive, so we don't
// try to maintain these semantics when manipulating a global over Xray (so the
// properties just re-resolve if they've been deleted).
//
// Unfortunately, there's a bit of an impedance-mismatch between the Xray and
// non-Xray machinery. The Xray machinery wants an API that returns a
// JSPropertyDescriptor, so that the resolve hook doesn't have to get snared up
// with trying to define a property on the Xray holder. At the same time, the
// DefineInterface callbacks are set up to define things directly on the global.
// And re-jiggering them to return property descriptors is tricky, because some
// DefineInterface callbacks define multiple things (like the Image() alias for
// HTMLImageElement).
//
// So the setup is as-follows:
// * The resolve function takes a JSPropertyDescriptor, but in the non-Xray case,
//   callees may define things directly on the global, and set the value on the
//   property descriptor to |undefined| to indicate that there's nothing more for
//   the caller to do. We assert against this behavior in the Xray case.
// * We make sure that we do a non-Xray resolve first, so that all the slots
//   are set up. In the Xray case, this means unwrapping and doing a non-Xray
//   resolve before doing the Xray resolve.
//
// This all could use some grand refactoring, but for now we just limp along.

@@ +2977,5 @@
>        if (!interfaceObject) {
>          return NS_ERROR_FAILURE;
>        }
>  
>        if (isXray) {

As mentioned in the previous patch, I think we should separate out the Xray and non-Xray cases into separate blocks, and get rid of the Maybe<JSAutoCompartment> stuff. This patch is probably the right place to do it.

@@ +3048,5 @@
>                       getter_AddRefs(proto_holder));
>  
>      if (NS_SUCCEEDED(rv) && obj != aWin->GetGlobalJSObject()) {
> +      MOZ_ASSERT(xpc::WrapperFactory::IsXrayWrapper(obj),
> +                 "What the heck is obj, exactly?");

How about:

rv = GetXPCProto(sXPConnect, cx, aWin, name_struct, geter_AddRefs(proto_holder));
NS_ENSURE_SUCCESS(rv, rv);
bool isXray = xpc::WrapperFactory::IsXrayWrapper(obj);
MOZ_ASSERT_IF(obj != aWin->GetGlobalJSObject(), isXray);
if (!isXray) {
  // GetXPCProto already defined the property for us.
  FillPropertyDescriptor(desc, obj, JS::UndefinedValue(), false);
  return NS_OK;
}

// Handle the Xray case via ResolvePrototype.

}

@@ +3066,3 @@
>      }
>  
> +    // GetXPCProto already defined the property for us

I can't immediately prove to myself that this is true, but I'll trust you on it.

@@ -3099,5 @@
>      rv = WrapNative(cx, obj, constructor, &NS_GET_IID(nsIDOMDOMConstructor),
>                      false, &val);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> -    rv = constructor->Install(cx, obj, val);

Shouldn't we also remove Install() from nsDOMConstructor? IIUC it's unused now, and its contents were moved into PostCreatePrototype.

@@ +3122,5 @@
>      NS_ASSERTION(val.isObject(), "Why didn't we get a JSObject?");
>  
> +    if (!JS_WrapValue(cx, &val)) {
> +      return NS_ERROR_UNEXPECTED;
> +    }

Same thing here - allow wrapping for the WrapNative() call, and kill the JS_WrapValue.

@@ +3449,5 @@
> +    // also already defined it, so we don't have to.
> +    if (desc.object() && !desc.value().isUndefined() &&
> +        !JS_DefinePropertyById(cx, global, id, desc.value(),
> +                               desc.getter(), desc.setter(),
> +                               desc.attributes())) {

So, we do this dance 3 in the different places now - once here, once below, and once in CGResolveOwnPropertyViaNewresolve. This seems fragile.

How about introducing a helper called GlobalResolveAndDefine, which asserts that its argument is non-Xray, does a GlobalResolve, and then does the define if needed? Then, this whole part at the bottom could just be:

JS::Rooted<JSObject*> global(cx, js::CheckedUnwrap(obj, false));
{
  JSAutoCompartment ac(cx, global);
  rv = GlobalResolveAndDefine(win, cx, global, id);
  NS_ENSURE_SUCCESS(rv, rv);
}
if (global != obj) {
  MOZ_ASSERT(IsXrayWrapper(obj));
  rv = GlobalResolve(win, cx, obj, id, &desc);
  NS_ENSURE_SUCCESS(rv, rv);
  // Do the legacy JS_DefinePropertyById to for XPCWN Xrays.
}

This handles both the Xray and non-Xray cases.

	Bobby Holley (:bholley)
	Comment 28 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #26)
> > I think we should rename it to 'getOrCreateInterfaceObject'.
> 
> Well, but it can define it too...  Peter, thoughts?

Yes, but the 'define' part is conditional on a boolean (which can be /* labeled = */), and doesn't happen in the Xray case (which is the whole reason this is confusing). The getOrCreate part is the truly inherent behavior.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 29 • 11 years ago

> How about just passing aAllowWrapping = true in the call to WrapNative above,

Gives the wrong behavior.  Note that I'm carefully wrapping ctorDesc.value(), not v.  v is used below there to create various other objects, and they need to end up in the compartment of |constructor|'s reflector, not in the compartment of cx.  I'll add a comment.

> I don't know if you have plans to do that at some point

I had no specific plans for it, but I'm happy to add comments, esp if you write them.  ;)

> I think we should separate out the Xray and non-Xray cases into separate blocks

OK.

> How about:

Seems reasonable.

> I can't immediately prove to myself that this is true, but I'll trust you on it.

It happens in PostCreatePrototype, afaict.

> Shouldn't we also remove Install() from nsDOMConstructor?

Yes.  Will do.

> Same thing here - allow wrapping for the WrapNative() call, and kill the JS_WrapValue.

Here we could do it.  If passing "true" for that 5th arg is equivalent to a post-fact JS_WrapValue, then I'll make that change.

> So, we do this dance 3 in the different places now - once here, once below, and once in
> CGResolveOwnPropertyViaNewresolve. This seems fragile.

I consider the code in nsWindowSH::NewResolve temporary; recall that the whole goal is to get rid of it in the next 2 months at most.  I can still do what you suggest, I guess, if you feel strongly about it.

One other note: I believe we in fact guarantee that in the xray case we will never return a desc with the undefined value.  Maybe I should just be asserting that...

	Bobby Holley (:bholley)
	Comment 30 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #29)
> I consider the code in nsWindowSH::NewResolve temporary; recall that the
> whole goal is to get rid of it in the next 2 months at most.  I can still do
> what you suggest, I guess, if you feel strongly about it.

I think it's worth doing, even if only for 2 months.
 
> One other note: I believe we in fact guarantee that in the xray case we will
> never return a desc with the undefined value.  Maybe I should just be
> asserting that...

Yes, let's do that.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 31 • 11 years ago

> I think it's worth doing, even if only for 2 months.

With the assert in the xray case, the code is actually slightly different between the cases... I'd really rather not introduce helper functions that we then have to remove later...

I've addressed the rest of the comments.

	Bobby Holley (:bholley)
	Comment 38 • 11 years ago

Comment on attachment 8364170 [details] [diff] [review]
Real interdiff for part 3

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

::: dom/base/nsDOMClassInfo.cpp
@@ +2969,5 @@
> +        {
> +          JSAutoCompartment ac(cx, global);
> +          interfaceObject = getOrCreateInterfaceObject(cx, global, id, false);
> +        }
> +        if (!interfaceObject) {

NS_WARN_IF, here and below? I really wish we could NS_ENSURE... :\

	Peter Van der Beken [:peterv]
	Comment 39 • 11 years ago

Comment on attachment 8364169 [details] [diff] [review]
Part 3, with the assert correct

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

::: dom/bindings/DOMJSProxyHandler.h
@@ +169,5 @@
> +FillPropertyDescriptor(JS::MutableHandle<JSPropertyDescriptor> desc,
> +                       JSObject* obj, unsigned attributes, JS::Value v)
> +{
> +  FillPropertyDescriptor(desc, obj, v, false);
> +  desc.setAttributes(attributes);

Doesn't it make more sense to inline what FillPropertyDescriptor does here, to avoid double-setting of attributes?

	Peter Van der Beken [:peterv]
	Comment 40 • 11 years ago

Comment on attachment 8363407 [details] [diff] [review]
part 6.  Fix the resolve/enumerate hooks in WebIDL bindings in the [Global] case to handle standard classes.

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

::: dom/bindings/Codegen.py
@@ +6022,5 @@
> +                    "bool did_resolve = false;\n"
> +                    "if (!JS_ResolveStandardClass(cx, obj, id, &did_resolve)) {\n"
> +                    "  return false;\n"
> +                    "}\n"
> +                    "if (did_resolve) {\n"

I think I'd prefer for this to call mozilla::dom::ResolveGlobal and return if objp is non-null.

@@ +6065,5 @@
> +    def definition_body(self):
> +        if self.descriptor.interface.getExtendedAttribute("Global"):
> +            # Enumerate standard classes
> +            prefix = CGIndenter(CGGeneric(
> +                    "if (!JS_EnumerateStandardClasses(cx, obj)) {\n"

And for this to call mozilla::dom::EnumerateGlobal.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 41 • 11 years ago

> Doesn't it make more sense to inline what FillPropertyDescriptor does here

Done.

> I think I'd prefer for this to call mozilla::dom::ResolveGlobal

Didn't know that existed!  Done, and for EnumerateGlobal.  Had to fix ResolveGlobal to take a Handle<id>, not a MutableHandle<id>, but I think the Handle is in fact correct.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 42 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/16fd42fa8fc8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6674a720f76f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e8bbbcff268
https://hg.mozilla.org/integration/mozilla-inbound/rev/6891009a669a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1275917b08fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/d34f91e55e75

	Ryan VanderMeulen [:RyanVM]
	Comment 43 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/16fd42fa8fc8
https://hg.mozilla.org/mozilla-central/rev/6674a720f76f
https://hg.mozilla.org/mozilla-central/rev/7e8bbbcff268
https://hg.mozilla.org/mozilla-central/rev/6891009a669a
https://hg.mozilla.org/mozilla-central/rev/1275917b08fe
https://hg.mozilla.org/mozilla-central/rev/d34f91e55e75