Closed Bug 861493 Opened 7 years ago Closed 7 years ago

Passing dictionaries from chrome to constructors in content doesn't work

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 + fixed
firefox23 + fixed

People

(Reporter: smaug, Assigned: bzbarsky)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 4 obsolete files)

Str:
- Open browser
- load about:blank
- open error console (ctrl+shitf+j or similar)
- run: var x = new top.opener.content.XMLHttpRequest({ mozAnon: true}); x.mozAnon;
  If you just run var x = new XMLHttpRequest({ mozAnon: true}); x.mozAnon; the value of
  mozAnon is right.

(Similar things happens with Event ctors)
Oh, maybe XHR mozAnon is special, but Event's properties certainly aren't.
var e = new top.opener.content.Event("test", {bubbles: true}); e.bubbles
vs.
var e = new Event("test", {bubbles: true}); e.bubbles
Hmm, no, XHR is checking the caller, so setting mozAnon when caller is chrome should work, but
doesn't.
Blocks: 847611
Obviously we get a ChromeObjectWrapper with no exposedProperties, so getting the dictionary object's properties fails.
(In reply to Peter Van der Beken [:peterv] from comment #4)
> Obviously we get a ChromeObjectWrapper with no exposedProperties, so getting
> the dictionary object's properties fails.

Do we? That would only happen if we didn't go over Xray (and entered the content compartment), right? Who's waiving in this case?
DOMXrayTraits::call/construct enter the content compartment and wrap the arguments in it.
So __exposedProps__ does help, but that is a new requirement. Do we want to force people to use that
even when chrome is just passing simple values within dictionary?
(In reply to Peter Van der Beken [:peterv] from comment #6)
> DOMXrayTraits::call/construct enter the content compartment and wrap the
> arguments in it.

Oh hm, that's not so great. So we more or less just lose Xray protections here? It seems like we should have a better story there. What are the exact constraints that are causing us to do this? Can we find other solutions?

(In reply to Olli Pettay [:smaug] from comment #7)
> So __exposedProps__ does help, but that is a new requirement. Do we want to
> force people to use that
> even when chrome is just passing simple values within dictionary?

Definitely not. __exposedProps__ (and content access of chrome JS in general) is deprecated, and we don't want to be depending on it for correctness.
> DOMXrayTraits::call/construct enter the content compartment and wrap the arguments in it.

Er... that's what I was proposing, I think.  So how did bug 853709 happen, then?

In any case, I can certainly change codegen to unsafe-unwrap dictionaries if we want to.  Or to only do it in certain cases (because otherwise it seems like you could exfiltrate cross-origin information by pretending your cross-origin Window is a dictionary in some API or something).
(In reply to Boris Zbarsky (:bz) from comment #9)
> > DOMXrayTraits::call/construct enter the content compartment and wrap the arguments in it.
> 
> Er... that's what I was proposing, I think.  So how did bug 853709 happen,
> then?

Presumably because File isn't on the new bindings, and this is for DOM Xray traits.

I'm _really not_ ok with this behavior though. :-(
 
> In any case, I can certainly change codegen to unsafe-unwrap dictionaries if
> we want to.  Or to only do it in certain cases (because otherwise it seems
> like you could exfiltrate cross-origin information by pretending your
> cross-origin Window is a dictionary in some API or something).

Case in point.
I believe we should fix this in Aurora too.
(In reply to Olli Pettay [:smaug] from comment #11)
> I believe we should fix this in Aurora too.

What is the user-affecting impact here? Comment 0 looks very edge casey.  If this has significant impact, who's going to take this?
We have some code, at least http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.jsm#383 which relies on the old behavior.
(I don't know whether some b2g will be released from 22)

peterv or bz will look at this, I believe.
> Comment 0 looks very edge casey.

Comment 0 are just simple steps to reproduce.  See comment 12 for an example of code that's broken right now.

In any case, I'm taking it, per meeting earlier today.
Assignee: nobody → bzbarsky
Attached patch m-c patchSplinter Review
The code this generates is just as horrible as you might think; I can attach what the test codegen ends up as if you want.

I'm not terribly happy about the MutableThis bits, but I couldn't think of any other sane way to do it.  :(
Attachment #741155 - Flags: review?(bobbyholley+bmo)
Attached patch Aurora patch (obsolete) — Splinter Review
The dom/ bits are all the same except for s/CheckedUnwrap/UnwrapObjectChecked/.  The xray bits had to be basically rewritten because the rooting setup and arguments are different on aurora, so those need to be read over carefully.
Attachment #741156 - Flags: review?(bobbyholley+bmo)
Comment on attachment 741155 [details] [diff] [review]
m-c patch

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

r=bholley with comments.

> I could break this up if desired, but I figured a single diff would be easier to land on both branches...

In the future, I'd prefer for the m-c patches to be broken up whenever possible (so that they're bisectable), and then just do the branch patches as a rollup. Not a big deal in this case though, since most of the scattered changes are clearly independent of each other.

::: dom/bindings/BindingDeclarations.h
@@ +259,5 @@
>    {
>      return mImpl.ref();
>    }
>  
> +  Optional& MutableThis() const

Mmm, "This" makes me think of |this| in a JS sense. How about ConstCast or AsMutable?

@@ +381,5 @@
>    {
>      return mValue;
>    }
>  
> +  JS::Value* operator&()

Hm, should this be .address() instead, to match the convention in the rooting API? Or do you actually need to pass these interchangeably with raw jsvals such that the different semantics would be a pain?

::: dom/bindings/BindingUtils.h
@@ +1433,5 @@
>    }
> +
> +  JSObject** operator&()
> +  {
> +    // Assert if we're empty, on purpose

Hm, but aren't we allowed to be empty? Isn't it valid to return an address of an uninitialized JSObject? Or do we want to disallow that here for some reason?

Looking at the Codegen.py changes, I get the impression that we could end up with null here, but maybe I'm missing something...

::: dom/bindings/Codegen.py
@@ +4026,5 @@
>  class MethodNotCreatorError(Exception):
>      def __init__(self, typename):
>          self.typename = typename
>  
> +sequenceWrapLevel = 0

This makes me :'( a little bit. When I first saw it, I thought it had to do with indentation, and it took me a bit to figure out what the deal was. Comment it?

In general, it seems like we need some kind of name allocator...

@@ +4035,5 @@
> +    "object", or spidermonkey-interface types inside return a CGThing
> +    that will wrap them into the current compartment.
> +    """
> +    if type.isAny():
> +        assert not type.nullable()

Are |any|s not nullable per-spec? Or is this just "we'll deal with them when they happen"?

@@ +4058,5 @@
> +        if type.nullable():
> +            type = type.inner
> +            value = "%s.MutableThis().Value()" % value
> +        global sequenceWrapLevel
> +        index = "i%d" % sequenceWrapLevel

I might call this "indexName"

@@ +4071,5 @@
> +                              (index, index, value, index)),
> +                         post="\n}")
> +
> +    if type.isDictionary():
> +        assert not type.nullable()

Don't support or per spec?

@@ +4090,5 @@
> +    if type.isUnion():
> +        raise TypeError("Can't handle wrapping of unions in constructor "
> +                        "arguments yet")
> +
> +    if (type.isString() or type.isPrimitive() or type.isEnum() or

For JS-implemented DOM APIs, we always do the full jsval->AString and AString->jsval conversion, right? If we short-circuited with jsval->jsval, this would be a problem...

@@ +4098,5 @@
> +
> +    raise TypeError("Unknown type; we don't know how to wrap it in constructor "
> +                    "arguments: %s" % type)
> +
> +def wrapMaybeOptionalTypeIntoCurrentCompartment(type, value, isOptional):

I _think_ it would be cleaner and more general to have a CGThing that handles optional values, and demultiplex the cases in the caller. The caller already has to check for isOptional when calling this, so I think it's probably a net win. Fee free to ignore if it turns out to be problematic, though.

So the callers would do:
wrap = wrapTypeIntoCurrentCompartment(..)
if (wrap and arg.optional and not arg.default_value)
  wrap = CGConditionalDefault(type, wrap)

or something.

@@ +4199,5 @@
>  
> +        if isConstructor:
> +            # If we're called via an xray, we need to enter the
> +            # underlying object's compartment and then wrap up all of
> +            # our arguments into that compartment as needed.

So the idea is that by the time this code has been invoked, things like Dictionaries and stuff will already have been parsed out of the incoming jsvals, right? And now we just need to manually emulate a CrossCompartmentWrapper?

This comment could use some beefing up with the answer to that question.

::: dom/bindings/Configuration.py
@@ +472,5 @@
>      """
>      members = [m for m in descriptor.interface.members]
>      if descriptor.interface.ctor():
>          members.append(descriptor.interface.ctor())
> +    members.extend(descriptor.interface.namedConstructors)

I'm going to take your word for this one.

::: dom/tests/mochitest/chrome/test_sandbox_bindings.xul
@@ +238,5 @@
> +        var e = new eventCtor("test", { bubbles: true });
> +        is(e.bubbles, true, "Dictionary argument should work");
> +      } catch (e) {
> +        ok(false, "Should be able to construct my event " + e);
> +      }

Hm, why does a sandbox need to be involved here? Shouldn't this also be testable just by doing:

new contentWindow.Event('test', { bubbles: true})

I'm all for this piece of test coverage, but I'd like us to test the simple and non-magical cases whenever we can, and anything that involves sandboxPrototype definitely doesn't fit that bill. :-)

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1234,5 @@
> +        vp[1] = args.thisv();
> +        PodCopy(vp.begin() + 2, args.array(), args.length());
> +        if (!clasp->call(cx, args.length(), vp.begin()))
> +            return false;
> +        args.rval().set(vp[0]);

Per IRC discussion, let's just replace this Pod stuff with .base().

@@ +1235,5 @@
> +        PodCopy(vp.begin() + 2, args.array(), args.length());
> +        if (!clasp->call(cx, args.length(), vp.begin()))
> +            return false;
> +        args.rval().set(vp[0]);
> +    } else {

I would prefer to structure this as:
// Long comment explaining what the heck we're doing with all this stuff. More or less the
// five-step process you dsecribed to me on IRC.
if (clasp->flags & JSCLASS_IS_DOMIFACEANDPROTOJSCLASS) {
  if (!clasp->call)
    throw;
} else {
// Long comment explaining how this can only be legacy webidl and how we really do
// want to enter the compartment there.
MOZ_ASSERT(IsLegacyWebIDL);
return Base::call().
}
Attachment #741155 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 741156 [details] [diff] [review]
Aurora patch

Can you incorporate the feedback from the m-c patch on this patch, and then flag that for review?
Attachment #741156 - Flags: review?(bobbyholley+bmo)
> Mmm, "This" makes me think of |this| in a JS sense. How about ConstCast or AsMutable?

I'll do AsMutable.

> Or do you actually need to pass these interchangeably with raw jsvals such that the
> different semantics would be a pain?

Exactly.  This allows me to just emit a & in the codegen for both raw values and these guys.

> Hm, but aren't we allowed to be empty?

In practice, not by the time the code that's using this operator runs.  The only reason it needs the ability to be empty is so it can have a default constructor, but right after that we init the dictionary members.

> Comment it?

Will do.

> Are |any|s not nullable per-spec?

Yes, since null is already a valid value for an "any".

> I might call this "indexName"

OK.

> Don't support or per spec?

Per spec, since null is already a valid value for a dictionary type.

> For JS-implemented DOM APIs, we always do the full jsval->AString and AString->jsval
> conversion, right?

Yes, absolutely, for exactly the reason you describe.

> So the callers would do:

Ah, hmm.  I'll see what that ends up looking like.

> So the idea is that by the time this code has been invoked, things like Dictionaries
> and stuff will already have been parsed out of the incoming jsvals, right?

Yes.  I'll improve the comments.

> Hm, why does a sandbox need to be involved here? Shouldn't this also be testable just
> by doing:
>   new contentWindow.Event('test', { bubbles: true})

Hmm.  Yes, I think so.  I mostly just added the test by copy-and-paste.  ;)

I'll try to add a direct test too.

>  if (!clasp->call)
>    throw;

Throwing the right exception for that case might be a bit of a pain.  I figured I'd just delegate it to whatever code normally throws them instead of trying to replicate it...

> MOZ_ASSERT(IsLegacyWebIDL);

I don't know how to assert this, honestly, short of saying that it's a WebIDL instance class with a call in the JSClass...
Jeff, can you look over the jsfriendapi bits?
Attachment #741994 - Flags: review?(jwalden+bmo)
Attached patch Updated aurora patch (obsolete) — Splinter Review
Attachment #742005 - Flags: review?(bobbyholley+bmo)
Attachment #741156 - Attachment is obsolete: true
Comment on attachment 742005 [details] [diff] [review]
Updated aurora patch

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

r=bholley with comments addressed (some of these probably apply to the mc patch too)

::: dom/tests/mochitest/chrome/test_xray_event_constructor.xul
@@ +21,5 @@
> +  <![CDATA[
> +  /** Test for Bug 861493 **/
> +   SimpleTest.waitForExplicitFinish();
> +
> +   addLoadEvent(function() {

Is the initial about:blank in the type=content iframe here guaranteed to get a content principal? Please add a ok(Cu.isXrayWrapper($("t").contentWindow) to be sure.

::: js/src/jsfriendapi.cpp
@@ +1058,5 @@
> +JS_FRIEND_API(JSBool)
> +js_ReportIsNotFunction(JSContext *cx, const JS::Value& v)
> +{
> +    return ReportIsNotFunction(cx, v);
> +}

Do we really need this wrapper? I you can just put the declaration of ReportIsNotFunction in jsfriendapi.h and call it a day.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +136,5 @@
>                                      JSObject *wrapper, JSObject *holder,
>                                      jsid id, JSPropertyDescriptor *desc, unsigned flags);
>  
> +    static bool call(JSContext *cx, JS::Handle<JSObject*> wrapper,
> +                     unsigned argc, Value *vp, js::Wrapper& baseSingleton)

Just call this |base|, or |baseInstance| if you prefer. All js::Wrappers are singletons.

@@ +1295,5 @@
> +        vp[1] = JS::UndefinedValue();
> +        PodCopy(vp.begin() + 2, argv, argc);
> +        if (!clasp->construct(cx, argc, vp.begin()) && vp[0].isObject())
> +            return false;
> +        rval.set(vp[0]);

Er, can't we get rid of the Pod stuff in this function too?
Attachment #742005 - Flags: review?(bobbyholley+bmo) → review+
> Is the initial about:blank in the type=content iframe here guaranteed to get a content
> principal?

Yes, but I'll add the check.

> I you can just put the declaration of ReportIsNotFunction in jsfriendapi.h and call it a
> day.

Its optional arguments pull in various types and enums and stuff...  I was trying to minimize how much I moved to friendapi.

> Er, can't we get rid of the Pod stuff in this function too?

No, we can't, because we really are just passed in a totally separate argv+argc and rval here, as far as I can tell.
Oh, I'll do baseInstance.
Whiteboard: [need review]
(In reply to Boris Zbarsky (:bz) from comment #24)
> No, we can't, because we really are just passed in a totally separate
> argv+argc and rval here, as far as I can tell.

Can you elaborate on this? The entry point from jsclass hook into proxy happens in proxy_{call,construct}, here:

http://mxr.mozilla.org/mozilla-central/source/js/src/jsproxy.cpp#3171

Both of them use CallArgsFromVp, and pass the result to Proxy::{call,construct}, which in turn passes them directly to the handler, which in turn passes them directly to the traits. How are the two cases different?
Attached patch Final aurora patch (obsolete) — Splinter Review
Attachment #742005 - Attachment is obsolete: true
Hmm.  Are we guaranteed that no one ever calls these handlers in any other way?  On Aurora, prox_Construct does in fact get the argc, argv, rval out of a JSNative's vp, so if we know that this code is _always_ only entered via proxy_Construct there we can in fact hold our noses and do argv-2...
Comment on attachment 741994 [details] [diff] [review]
Patch addressing comments

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

Yeah, not exposing those hairy trailing arguments seems fairly clearly the best thing to do.
Attachment #741994 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d85f6cc0eb4b for the trunk bits, since everyone is happy with them.
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla23
(In reply to Boris Zbarsky (:bz) from comment #28)
> Hmm.  Are we guaranteed that no one ever calls these handlers in any other
> way?  On Aurora, prox_Construct does in fact get the argc, argv, rval out of
> a JSNative's vp, so if we know that this code is _always_ only entered via
> proxy_Construct there we can in fact hold our noses and do argv-2...

We know that it always gets invoked by Proxy::construct. It's possible that somebody else might call that (rather than the JSClass hook), but currently they don't. Can somebody actually create a CallArgs that doesn't cast back to vp?
> Can somebody actually create a CallArgs that doesn't cast back to vp?

No, but on aurora what we have isn't a CallArgs, just a bunch of separate arguments....
OK, I talked this over with Bobby.  I'm going to make this change from the last aurora construct() in this bug:

-        JS::AutoValueVector vp(cx);
-        if (!vp.resize(2 + argc))
-            return false;
-        vp[0] = JS::ObjectValue(*wrapper);
-        vp[1] = JS::UndefinedValue();
-        PodCopy(vp.begin() + 2, argv, argc);
-        if (!clasp->construct(cx, argc, vp.begin()) && vp[0].isObject())
+        // We're going to assume that argv == vp + 2
+        JS::Value* vp = argv - 2;
+        if (!clasp->construct(cx, argc, vp))
Attached patch Final Aurora patch (obsolete) — Splinter Review
Attachment #742038 - Attachment is obsolete: true
Attachment #742117 - Attachment is obsolete: true
Comment on attachment 742119 [details] [diff] [review]
Final Aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Several things together, but the most
   recent is bug 822399
User impact if declined: Extension compat issues due to event constructors not
   working right in chrome code.
Testing completed (on m-c, etc.): Passes try.
Risk to taking this patch (and alternatives if risky): I think the risk is
   medium.  I'm pretty sure this is safe and all, and that we didn't miss
   anything in the value wrapping code we're adding.  But I'm not sure I'd stake
   my life on it...  The alternative is probably to ship the broken constructors.
String or IDL/UUID changes made by this patch: None.
Attachment #742119 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d85f6cc0eb4b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 742119 [details] [diff] [review]
Final Aurora patch

If we suspect we'll need an add-on compat regression fix in FF22, we should make sure to land as early as possible. Given that, approving.
Attachment #742119 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
is there any manual verification needed, considering the automated test that is available
Probably not, though the steps in comment 0 are the way to do it if you want to verify.
Marking [qa-] as per comment 41. Please remove this whiteboard tag and add the qawanted keyword if some testing is needed.
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.