Closed Bug 740069 Opened 12 years ago Closed 12 years ago

Land Paris Bindings and use them for XMLHttpRequest


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

Not set





(Reporter: khuey, Assigned: peterv)


(Depends on 2 open bugs, Blocks 1 open bug)


(Keywords: dev-doc-needed, Whiteboard: Landing from repo)


(9 files, 2 obsolete files)

      No description provided.
Attached patch Current patch (obsolete) — Splinter Review
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.
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.
Depends on: 737911
Depends on: 740469
And r+ for peterv's changes to nsDOMEventTargetHelper.
Depends on: 740526
Attachment #610438 - Attachment description: Patch without the random XHR whitespace and hgtags changes → Patch without the random XHR whitespace and hgtags changes. Generated with diff -r a5c5bc683072 -r 0a5bb7203953
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 on attachment 610726 [details]
review comments up to XrayWrapper

>--- a/js/xpconnect/src/nsXPConnect.cpp
>+++ b/js/xpconnect/src/nsXPConnect.cpp
>-    *_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 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.
I've updated to all of Olli's comments except the one about namespace overuse.  I'll file a followup bug on that.
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.
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 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/
@@ +23,5 @@
> +        self.interfaces = {}
> +        for iface in parseData:
> +            if not iface.isInterface(): continue
> +            self.interfaces[] = iface
> +            if 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;


@@ +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/
@@ +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 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 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


> > +
> >  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.


> 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).
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()
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 741163
Assignee: nobody → peterv
Flags: in-testsuite+
Depends on: 741263
Depends on: 741367
Depends on: 741248
Blocks: 742152
Depends on: 742156
Depends on: 742197
Filed the bugs in my followup list; mostly blocking bug 580070.
Depends on: 742217
Depends on: 742222
Blocks: 622300
We should get some documentation about this code up.
Keywords: dev-doc-needed
I'm working on a blog post that will explain how these new bindings work.
(In reply to Johnny Stenback (:jst, 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.
Depends on: 743376
Summary: Land Paris Bindings → Land Paris Bindings and use them for XMLHttpRequest
Depends on: 743666
Depends on: 748983
Depends on: 749774
Depends on: 753642
Depends on: 756173
Depends on: CVE-2012-3963
Depends on: 765704
Depends on: 776239
Depends on: 775543
Depends on: 778152
Depends on: CVE-2012-3989
Depends on: 780529
Depends on: CVE-2012-4208
Depends on: 800386
Depends on: 830399
Depends on: 880196
Whiteboard: Landing from repo
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.