Last Comment Bug 740069 - Land Paris Bindings and use them for XMLHttpRequest
: Land Paris Bindings and use them for XMLHttpRequest
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
: 443904 (view as bug list)
Depends on: 741157 749774 737911 740469 740526 741125 741163 741248 741263 741267 741367 741390 742156 742197 742217 742222 743376 743666 744339 744772 748983 753642 756173 CVE-2012-3963 765704 775543 776239 778152 780529 CVE-2012-3989 CVE-2012-4208 800386 830399 880196 965468
Blocks: ParisBindings 622300 697271 742152
  Show dependency treegraph
 
Reported: 2012-03-28 11:31 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2014-01-29 17:37 PST (History)
21 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Current patch (1000.96 KB, patch)
2012-03-28 17:25 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
List of files changed in the patch (3.16 KB, text/plain)
2012-03-28 17:27 PDT, Boris Zbarsky [:bz]
no flags Details
Patch without the random XHR whitespace and hgtags changes. Generated with diff -r a5c5bc683072 -r 0a5bb7203953 (993.40 KB, patch)
2012-03-28 22:43 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
comments for content/ and dom/base (6.64 KB, text/plain)
2012-03-29 11:01 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details
Comments up to codegen.py (57.53 KB, text/plain)
2012-03-29 11:27 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details
review comments up to XrayWrapper (5.84 KB, text/plain)
2012-03-29 15:34 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details
Xray wrapper review (8.48 KB, text/plain)
2012-03-29 21:27 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details
Halfway through CodeGen.py (16.45 KB, text/plain)
2012-03-29 23:37 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details
The rest of CodeGen.py (3.04 KB, text/plain)
2012-03-30 16:12 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details
List of things we need followup bugs about (3.33 KB, text/plain)
2012-03-30 18:01 PDT, Boris Zbarsky [:bz]
no flags Details
Followup bug list with an extraneous item removed (3.09 KB, text/plain)
2012-03-30 18:03 PDT, Boris Zbarsky [:bz]
no flags Details

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-28 11:31:10 PDT

    
Comment 1 Boris Zbarsky [:bz] 2012-03-28 17:25:55 PDT
Created attachment 610373 [details] [diff] [review]
Current patch
Comment 2 Boris Zbarsky [:bz] 2012-03-28 17:27:23 PDT
Created attachment 610374 [details]
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 [:bz] 2012-03-28 18:33:29 PDT
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 [:bz] 2012-03-28 22:43:40 PDT
Created attachment 610438 [details] [diff] [review]
Patch without the random XHR whitespace and hgtags changes.  Generated with diff -r a5c5bc683072 -r 0a5bb7203953
Comment 5 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-03-29 11:01:20 PDT
Created attachment 610605 [details]
comments for content/ and dom/base
Comment 6 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-03-29 11:15:10 PDT
And r+ for peterv's changes to nsDOMEventTargetHelper.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-29 11:27:36 PDT
Created attachment 610613 [details]
Comments up to codegen.py
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-03-29 15:34:25 PDT
Created attachment 610726 [details]
review comments up to XrayWrapper
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-03-29 17:22:52 PDT
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) (busy with Stylo) 2012-03-29 21:27:57 PDT
Created attachment 610804 [details]
Xray wrapper review
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2012-03-29 23:25:06 PDT
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 (⌚ UTC+1/+2) 2012-03-29 23:28:14 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-29 23:37:37 PDT
Created attachment 610811 [details]
Halfway through CodeGen.py
Comment 14 Boris Zbarsky [:bz] 2012-03-30 00:29:28 PDT
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 [:bz] 2012-03-30 01:22:29 PDT
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 [:bz] 2012-03-30 02:38:56 PDT
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!) 2012-03-30 10:55:20 PDT
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!) 2012-03-30 12:06:39 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-30 16:12:05 PDT
Created attachment 611070 [details]
The rest of CodeGen.py
Comment 20 Peter Van der Beken [:peterv] 2012-03-30 17:03:02 PDT
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) 2012-03-30 17:53:51 PDT
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 [:bz] 2012-03-30 18:01:11 PDT
Created attachment 611099 [details]
List of things we need followup bugs about
Comment 23 Boris Zbarsky [:bz] 2012-03-30 18:03:00 PDT
Created attachment 611100 [details]
Followup bug list with an extraneous item removed
Comment 24 Peter Van der Beken [:peterv] 2012-03-30 22:18:55 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bdb337e3136
Comment 25 Ed Morley [:emorley] 2012-03-31 19:19:49 PDT
https://hg.mozilla.org/mozilla-central/rev/1bdb337e3136
Comment 26 Boris Zbarsky [:bz] 2012-04-03 22:25:50 PDT
Filed the bugs in my followup list; mostly blocking bug 580070.
Comment 27 :Ms2ger (⌚ UTC+1/+2) 2012-04-06 11:37:52 PDT
We should get some documentation about this code up.
Comment 28 Eric Shepherd [:sheppy] 2012-04-06 11:42:18 PDT
For docs, see: https://etherpad.mozilla.org/dom-bindings
Comment 29 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-06 13:25:57 PDT
I'm working on a blog post that will explain how these new bindings work.
Comment 30 Eric Shepherd [:sheppy] 2012-04-06 13:26:59 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) 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.
Comment 31 David Bruant 2012-12-12 06:19:06 PST
*** Bug 443904 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.