Land Paris Bindings and use them for XMLHttpRequest

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: khuey, Assigned: peterv)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla14
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 2 obsolete attachments)

Comment hidden (empty)

Comment 1

5 years ago
Created attachment 610373 [details] [diff] [review]
Current patch

Comment 2

5 years ago
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

5 years ago
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

5 years ago
Created attachment 610438 [details] [diff] [review]
Patch without the random XHR whitespace and hgtags changes.  Generated with diff -r a5c5bc683072 -r 0a5bb7203953
Attachment #610373 - Attachment is obsolete: true

Updated

5 years ago
Depends on: 737911
Depends on: 740469
Created attachment 610605 [details]
comments for content/ and dom/base
And r+ for peterv's changes to nsDOMEventTargetHelper.
Created attachment 610613 [details]
Comments up to codegen.py

Updated

5 years ago
Depends on: 740526

Updated

5 years ago
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
Created attachment 610726 [details]
review comments up to XrayWrapper
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.
Created attachment 610804 [details]
Xray wrapper review
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 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.
Created attachment 610811 [details]
Halfway through CodeGen.py

Comment 14

5 years ago
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

5 years ago
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

5 years ago
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/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 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.
Created attachment 611070 [details]
The rest of CodeGen.py
(Assignee)

Comment 20

5 years ago
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
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

5 years ago
Created attachment 611099 [details]
List of things we need followup bugs about

Comment 23

5 years ago
Created attachment 611100 [details]
Followup bug list with an extraneous item removed
Attachment #611099 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bdb337e3136
Depends on: 741125
https://hg.mozilla.org/mozilla-central/rev/1bdb337e3136
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 741157

Updated

5 years ago
Depends on: 741163
Assignee: nobody → peterv
Flags: in-testsuite+
Depends on: 741263
Depends on: 741267

Updated

5 years ago
Depends on: 741367

Updated

5 years ago
Depends on: 741390
Depends on: 741248

Updated

5 years ago
Blocks: 742152

Updated

5 years ago
Depends on: 742156

Updated

5 years ago
Depends on: 742197

Comment 26

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

Updated

5 years ago
Depends on: 742217

Updated

5 years ago
Depends on: 742222

Updated

5 years ago
Blocks: 622300
We should get some documentation about this code up.
Keywords: dev-doc-needed
For docs, see: https://etherpad.mozilla.org/dom-bindings
I'm working on a blog post that will explain how these new bindings work.
(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.

Updated

5 years ago
Depends on: 743376

Updated

5 years ago
Summary: Land Paris Bindings → Land Paris Bindings and use them for XMLHttpRequest

Updated

5 years ago
Depends on: 743666
Depends on: 744339
Depends on: 744772

Updated

5 years ago
Blocks: 697271

Updated

5 years ago
Depends on: 748983
Depends on: 749774

Updated

5 years ago
Depends on: 753642

Updated

5 years ago
Depends on: 756173

Updated

5 years ago
Depends on: 762280

Updated

5 years ago
Depends on: 765704

Updated

5 years ago
Depends on: 776239

Updated

5 years ago
Depends on: 775543

Updated

5 years ago
Depends on: 778152

Updated

5 years ago
Depends on: 783867

Updated

5 years ago
Depends on: 780529
Depends on: 798264
Depends on: 800386

Updated

5 years ago
Duplicate of this bug: 443904
Depends on: 830399

Updated

4 years ago
Depends on: 880196
Depends on: 965468
You need to log in before you can comment on or make changes to this bug.