740069 - Land Paris Bindings and use them for XMLHttpRequest

Status: RESOLVED FIXED

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Current patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: List of files changed in the patch

This is a list of files, along with who should review what.  Please put comments in this bug, possibly in attachments.  The "TAKE OUT" files are things we should not land yet, probably.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The diff in comment 1 was generated by diffing rev 9e606ca691a5 against rev a5c5bc683072.  So if any more changes land in the dombindings repo that you want reviewed, please post them as diffs against rev 9e606ca691a5 or against the previous thing that got attached.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch without the random XHR whitespace and hgtags changes. Generated with diff -r a5c5bc683072 -r 0a5bb7203953

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: comments for content/ and dom/base

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

And r+ for peterv's changes to nsDOMEventTargetHelper.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Comments up to codegen.py

Comment 8

[Bobby Holley (:bholley)]

Attachment: review comments up to XrayWrapper

Comment 9

[Bobby Holley (:bholley)]

Also, FWIW, my use of r- has been 'somebody absolutely needs to fix this before the patch is landed'. It doesn't always mean I have to look at it again. Use judgement.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Xray wrapper review

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 610726 [details]
review comments up to XrayWrapper

>--- a/js/xpconnect/src/nsXPConnect.cpp
>+++ b/js/xpconnect/src/nsXPConnect.cpp
>
>r=bholley
>
>-    *_retval = wrappedGlobal.forget().get();
>+    // Stuff coming through this path always ends up as a DOM global.
>+    // XXX Someone who knows why we can assert this should re-check
>+    //     (after bug 720580).
>+    MOZ_ASSERT(js::GetObjectClass(global)->flags & JSCLASS_DOM_GLOBAL);
>+    mozilla::dom::bindings::AllocateProtoOrIfaceCache(global);
>
>I can confirm that this is ok - the comment here should be removed.

Thanks; your changes confused me while merging.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 610804 [details]
Xray wrapper review

>--- a/js/xpconnect/wrappers/XrayWrapper.cpp
>+++ b/js/xpconnect/wrappers/XrayWrapper.cpp
>> +
>> +bool
>> +DOMXrayTraits::resolveOwnProperty(JSContext *cx, js::Wrapper &jsWrapper, JSObject *wrapper,
>> +                                  JSObject *holder, jsid id, bool set, JSPropertyDescriptor *desc)
>> +{
>> +    JSObject *obj = getInnerObject(wrapper);
>> +    DOMClass *domClass = &DOMJSClass::FromJSClass(JS_GetClass(obj))->mClass;
>> +
>> +    bool ok = domClass->mResolveProperty(cx, wrapper, id, set, desc);
>
>Is this consistent with the notion of |own| properties in WebIDL? I confess to not knowing
>the spec well. Are properties and methods of the most-derived class supposed to live on the
>object, or some sort of shared prototype (like they do now)? I'm assuming they live on the
>object. But if they don't, this is probably incorrect.

All attributes/methods should be on the prototype per spec, yes.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Halfway through CodeGen.py

Comment 14

[Boris Zbarsky [:bzbarsky]]

I've updated to all of Olli's comments except the one about namespace overuse.  I'll file a followup bug on that.

Comment 15

[Boris Zbarsky [:bzbarsky]]

The SSM changes khuey was asking about are OK.  They're a slight performance deoptimization for a rare case, and that code should be going away with compartment-per-global.  We discussed the reason the double-negative he mentions is needed.  Applied the BindingGen changes and Bindings.conf changes.  InputStream is used in XHR.send(), so we do in fact need it.  I believe we need the IID thing on workers because XHR has a getInterface, even on workers.  I'll file a followup bug on being able to mark idl stuff as mainthreadonly. 

We apparently use the declareOnly thing to have something only appear in the header, and in the cases when it's used child.define() is not in fact empty; it's equal to child.declare()....

The finalizer code is in fact what the current code does.

The JS object we're wrapping can implement QI, and return different results on different calls.  Yes, this is dumb.

Will file a followup bug on nixing the QueryInterface.  Same for using an enum in PropertyArrays.__init__.  Same for using CGIndenter in CGNativeToSupportsMethod.  Same for converting argument unwrapping to CGThings.  Likewise for the int64/uint64 conversions.

Applied the other comments from the comment 13 review.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Actually, I undid the comment about using "with", since that fails to build on Android.  I think the Python version there is too old.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 610438 [details] [diff] [review]
Patch without the random XHR whitespace and hgtags changes.  Generated with diff -r a5c5bc683072 -r 0a5bb7203953

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

::: dom/bindings/Configuration.py
@@ +23,5 @@
> +        self.interfaces = {}
> +        for iface in parseData:
> +            if not iface.isInterface(): continue
> +            self.interfaces[iface.identifier.name] = iface
> +            if iface.identifier.name not in config: continue

Seems like we should continue without adding iface to self.interfaces if it's not in config, right?

@@ +88,5 @@
> +
> +        castableDefault = not self.interface.isCallback()
> +        self.castable = desc.get('castable', castableDefault)
> +
> +        self.notflattened = desc.get('notflattened', False)

We need a better annotation name here...

@@ +102,5 @@
> +
> +        self.prefable = desc.get('prefable', False)
> +
> +        self.workers = desc.get('workers', False)
> +        self.nativeIsISupports = not self.workers

We'll need to make sure that this checks the config, apparently there are some future main-thread cases that peterv is thinking about which may not be nsISupports.

@@ +132,5 @@
> +
> +        for attribute in ['infallible', 'implicitJSContext', 'resultNotAddRefed']:
> +            addExtendedAttribute(attribute, desc.get(attribute, {}))
> +
> +        self.binaryNames = desc.get('binaryNames', {})

We don't use this any more. Remove?

::: dom/bindings/DOMJSClass.h
@@ +56,5 @@
> +
> +  // We cache the VTable index of GetWrapperCache for objects that support it.
> +  //
> +  // -1 indicates that GetWrapperCache is not implemented on the underlying object.
> +  const int16_t mGetWrapperCacheVTableOffset;

Unused.

@@ +61,5 @@
> +
> +  // We store the DOM object in reserved slot DOM_OBJECT_SLOT.
> +  // Sometimes it's an nsISupports and sometimes it's not; this class tells
> +  // us which it is.
> +  const bool mDOMObjectIsISupports;

This only seems to be used on the main thread in xpc/caps (where it should always be true). Maybe we can figure out a better way to convey this later.

::: dom/bindings/GlobalGen.py
@@ +53,5 @@
> +    # Parse the WebIDL.
> +    parser = WebIDL.Parser()
> +    for filename in fileList:
> +        fullPath = os.path.normpath(os.path.join(baseDir, filename))
> +        f = open(fullPath, 'r')

This should be in binary mode unless we have a good reason not to do it.

::: dom/bindings/Utils.h
@@ +20,5 @@
> +namespace dom {
> +namespace bindings {
> +
> +inline bool
> +Throw(JSContext* cx, nsresult rv)

We need to throw real DOM exceptions here. Need to do a followup ASAP.

@@ +59,5 @@
> +
> +  JS::Value val = js::GetReservedSlot(obj, slot);
> +  // XXXbz worker code tries to unwrap things that aren't wrapping
> +  // anything for some reason!  We need to make it stop and remove
> +  // this check.

Update comment to reflect reality, workers do this for interface objects and need to stop.

::: dom/workers/WorkerScope.cpp
@@ +708,2 @@
>  
> +    mozilla::dom::bindings::AllocateProtoOrIfaceCache(aObj);

Need to make sure we free this!

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 610438 [details] [diff] [review]
Patch without the random XHR whitespace and hgtags changes.  Generated with diff -r a5c5bc683072 -r 0a5bb7203953

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

::: dom/bindings/Utils.cpp
@@ +53,5 @@
> +    return NULL;
> +  }
> +
> +  if (!JS_DefineProperty(cx, global, name, OBJECT_TO_JSVAL(constructor), NULL,
> +                         NULL, 0)) {

Maybe want a comment here saying that it's intentionally not enumerable per spec.

@@ +123,5 @@
> +      return NULL;
> +    }
> +  }
> +
> +  return protoClass ? proto : interface;

Should comment why you return one or the other (caching on global).

::: dom/bindings/Utils.h
@@ +309,5 @@
> +    *ok = false;
> +    return 0;
> +  }
> +  int i = 0;
> +  for (const EnumEntry* value = values; value->value; ++value, ++i) {

We should be able to get the compiler to tell us how many elements a particular EnumEntry[] has in it rather than null checking each time. Small win, fine to wait til later.

@@ +317,5 @@
> +
> +    bool equal = true;
> +    const char* val = value->value;
> +    for (size_t j = 0; j != length; ++j) {
> +      if (unsigned(val[j]) != unsigned(chars[j])) {

Maybe someday we can generate uchars!

@@ +385,5 @@
> +}
> +
> +template<class T>
> +inline bool
> +WrapObject(JSContext* cx, JSObject* scope, nsCOMPtr<T> &p, const nsIID* iid,

Followup: use template<template<class>> trick to reduce nsCOMPtr/nsRefPtr duplication here.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: The rest of CodeGen.py

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 610804 [details]
Xray wrapper review

> These symbols, especially 'Proxy', seem likely to conflict with other things.
> Can we prefix the values or namespace the enum?

I've used

    XrayForDOMObject,
    XrayForDOMProxyObject,
    XrayForWrappedNative,
    NotXray

> 
> > +
> >  static bool
> > -CanXray(JSObject *obj, bool *proxy)
> > +CanXray(JSObject *obj, XrayType *type)
> 
> Now that we have an enum, this doesn't need to have a funky in-param. Just add
> |None| to the enum and make CanXray return an XrayType. At that point, the
> method should be renamed to something like GetXrayType.

It's a little annoying for things like

  |targetdata && targetdata->wantXrays && CanXray(obj, &type)|

but done.

> 
> > +    JSClass* clazz = JS_GetClass(obj);
> 
> clazz -> clasp.

XPConnect uses both, but done.

> I kind of wish you'd gone with the eager creation scheme rather than taking
> us further down this path. I guess we'll fix it later, though.

Later.

> So, I think that _all_ of this needs to be in the common code, not just wrappedJSObject.
> I guess we can get away with it for now since they only apply to nodes (and XHR isn't
> a node), but we need a bug on it.

Yes, follwoup.

> Also, this monstrosity of a predicate needs to turn into a helper method with comments
> before landing. Not kidding.

We talked this through, followup. Not kidding either.

> Aren't we expected to fill in empty values in the JSPropertyDescriptor if we didn't find anything?
> That's what GetPropertyDescriptorById in jsapi.cpp does. It's not at all clear to me that we can
> assume desc to be zeroed on entry, and with the current code, XrayWrapper::getPropertyDescriptor
> depends on it when it calls this function.

I think it's the caller's responsibility to null out. Isn't that why JSAPI does it in the API, so we don't have to when we're called from it?

> Is this consistent with the notion of |own| properties in WebIDL? I confess to not knowing
> the spec well. Are properties and methods of the most-derived class supposed to live on the
> object, or some sort of shared prototype (like they do now)? I'm assuming they live on the
> object. But if they don't, this is probably incorrect.

Yeah, this was wrong. Done.

> Doesn't this overwrite expandos? The contract seems to be that it's
> resolveOwnProperty's responsibility to check for pre-existing properties before
> calling through to native stuff. I don't see how expandos could work at all here,
> really.

Removed because of previous comment.

> Doesn't bp point to garbage at this point? And don't we need to fill it in somewhere?
> I'm assuming this is just a partial copy-paste of the Proxy stuff.

Removed the check for *bp.

> 
> > +bool
> > +DOMXrayTraits::enumerateNames(JSContext *cx, JSObject *wrapper, unsigned flags,
> > +                              JS::AutoIdVector &props)
> > +{
> > +    if (flags & (JSITER_OWNONLY | JSITER_HIDDEN))
> > +        // Probably need to return expandos on the Xray here!
> 
> Yeah, we do. But we probably can't do that until we have a separate expando object.

Yes, followup.

> But given how resolveOwnProperty works, my assumption is that |own| properties
> include those of the most-derived class. In that case, don't we need to enumerate
> those here? And if they don't, then resolveOwnProperty needs to be fixed.

resolveOwnProperty was fixed.

> > +    unsigned flags = (set ? JSRESOLVE_ASSIGNING : 0) | JSRESOLVE_QUALIFIED;
> 
> What does this do? It certainly deserves a comment..

It sets the right flags to pass to JS_GetPropertyDescriptorById. The flags are documented in jsapi.h.

> >      return JS_DefinePropertyById(cx, holder, id, desc->value, desc->getter, desc->setter,
> > -                                 desc->attrs);
> > +                                 desc->attrs) &&
> > +           JS_GetPropertyDescriptorById(cx, holder, id, flags, desc);
> 
> Why doesn't this need to happen in getOwnPropertyDescriptor as well?

This was redone, we don't need the JS_GetPropertyDescriptorById after all (caller should do that).

https://hg.mozilla.org/try/rev/391af35c5ab7

Comment 21

[Justin Lebar (not reading bugmail)]

I have a bunch of relatively benign fixes to the parser, but here's a correctness fix we should make sooner rather than later.

In IDLIdentifier,

    def __eq__(self, other):
        return self.QName == other.QName()

should be

    def __eq__(self, other):
        return self.QName() == other.QName()

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: List of things we need followup bugs about

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Followup bug list with an extraneous item removed

Comment 24

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bdb337e3136

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1bdb337e3136

Comment 26

[Boris Zbarsky [:bzbarsky]]

Filed the bugs in my followup list; mostly blocking bug 580070.

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We should get some documentation about this code up.

Comment 28

[Eric Shepherd [:sheppy]]

For docs, see: https://etherpad.mozilla.org/dom-bindings

Comment 29

[Johnny Stenback (:jst)]

I'm working on a blog post that will explain how these new bindings work.

Comment 30

[Eric Shepherd [:sheppy]]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #29)
> I'm working on a blog post that will explain how these new bindings work.

Awesome; if you could add a link to it in a comment here that would be a big help.