778152 - Content window does not have an XMLHttpRequest property when accessed via an Xray wrapper in a subscript

Status: RESOLVED FIXED

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

Description

[Matthew Gertner]

Attachment: Extension that reproduces the issue

We have a simple extension framework that lets the user create content scripts that run with chrome privileges but share the DOM with a webpage (similar to the Chrome extension API or Mozilla Addon SDK).

Since FF14 we are no longer able to make XMLHttpRequests using jQuery from content scripts. Apparently the window object (i.e. gBrowser.contentWindow) does not have an XMLHttpRequest property when accessed from the content script (which is loaded using the subscript loader).

The attached extension illustrates the problem. It works fine in FF13 (you can see the "success" logs written to the console using dump). In FF14 jQuery complains that "window.XMLHttpRequest is not a constructor". I checked and window.XMLHttpRequest is null at this point in the code.

Comment 1

[Matthew Gertner]

Sorry, it's been a long day. The value of window.XMLHttpRequest is undefined, not null.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I get things like:

--- SUCCESS: [object Object]

with the attached extension and a current debug build...

Comment 3

[Matthew Gertner]

Current means trunk?

Comment 4

[Boris Zbarsky [:bzbarsky]]

But in a current nightly I do have the problem, apparently.  Weird.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yes, current means trunk.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This is bizarre.  So I added logging to the ajax() function in the subscript, like so:

  dump("\n\n\nMYTEST: " + window.XMLHttpRequest + "\n\n\n")

at the entry point to the function.  When I do that, in today's nightly I see this when starting the browser (whitespace slightly trimmed):

  MYTEST: function XMLHttpRequest() {
      [native code]
  }
  MYTEST: function XMLHttpRequest() {
      [native code]
  }
  --- SUCCESS: [object Object]
  --- SUCCESS: [object Object]
  *** JQUERY: TypeError: window.XMLHttpRequest is not a constructor
  MYTEST: undefined
  --- ERROR: No Transport

(note that I start it with several tabs open).  Then every time I open a new window I get one instance of:

  MYTEST: function XMLHttpRequest() {
      [native code]
  }
  --- SUCCESS: [object Object]

I a debug build I don't get the typerror/undefined/no-transport bits.  And I don't get anything printed when opening a window (!).

In Firefox 14 I get:

  MYTEST: function XMLHttpRequest() {
      [native code]
  }
  --- SUCCESS: [object Object]
  *** JQUERY: TypeError: window.XMLHttpRequest is not a constructor
  MYTEST: undefined
  --- ERROR: No Transport

and then every tab open or window open acts like in a current nightly.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, interesting.  With a bit more logging, that looks like so:

  MYTEST: [object Window], about:blank, function XMLHttpRequest() {
      [native code]
  }
  MYTEST: [object Window], about:blank, function XMLHttpRequest() {
      [native code]
  }
  MYTEST: [object Window], http://www.mozilla.org/en-US/firefox/channel/#aurora/aurora-desktop, undefined

Comment 8

[Boris Zbarsky [:bzbarsky]]

So easy way to reproduce: set browser.newtab.url to something like http://www.mozilla.org, then open a tab, with the attached extension installed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So I just did a quick test and normal Xrays do show XMLHttpRequest, but not random expandos.  So this is not purely an Xray problem....

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, wait.  So I bet the key issue here really is the subscript plus xray stuff somehow.  In particular, after getting window.XMLHttpRequest I get null but this _does_ define XMLHttpRequest on the _global_, just not on the _window_.

Peter, this sounds like something similar to the sandboxing bits you fixed...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ok, so I finally reproduced in a debug build.  There I also get:

###!!! ASSERTION: id got defined somewhere else?: 'JS_HasPropertyById(cx, holder, id, &hasProp) && hasProp', file ../../../../mozilla/js/xpconnect/wrappers/XrayWrapper.cpp, line 867

Comment 12

[Boris Zbarsky [:bzbarsky]]

So when we land in mozilla::dom::XMLHttpRequestBinding::DefineDOMInterface we have the following:

aReceiver is a proxy of class js::ObjectProxyClass.  Its handler is a xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::XPCWrappedNativeXrayTraits>.  Its wrapped object is itself a proxy whose handler is nsOuterWindowProxy.

"global" ends up as a ChromeWindow.

So we enter XMLHttpRequestBinding::GetProtoObject with aGlobal as the ChromeWindow and aReceiver as the Xray.

We look up the protoOrIfaceArray on the global (the ChromeWindow, remember?).  It already has a cached prototype object for this proto id.  So we just return that.  And we never actually define the property on the underlying content Window object.  Then we unwind to Xray's getPropertyDescriptor, desc->obj is false, we have nothing on our holder, resolveNativeProperty finds nothing, and we return.

That whole global/receiver/whatever thing is from bug 741267.

So the real problem is that we're landing in DefineDOMInterface with the following situation:

  1)  The receiver is an Xray around a window, not an actual window.
  2)  The global of the receiver _is_ a window (albeit a different one!)

I _think_ that for non-xray cross-compartment proxies we get more transparent forwarding, so we'd land in DefineDOMInterface with the wrapped object of the cross-compartment proxy, not with the proxy itself.  And then the global would be correct...

Does it make any sense to unwrap Xrays here before looking up the global?  There are obvious issues about compartment mismatches and whatnot, but the current setup is just broken in this situation.  Of course it doesn't help that the subscript loader just runs everything against the chrome window global.... ;)

Comment 13

[Matthew Gertner]

Note that in my actual extension I am frequently getting a failure at:


#0	0x000000010950d4e4 in nsXMLHttpRequest::_Constructor(nsISupports*, unsigned int&) at /Users/matthewgertner/Development/mozrelease/src/content/base/src/nsXMLHttpRequest.h:219
#1	0x0000000109509849 in mozilla::dom::bindings::prototypes::XMLHttpRequest::_Construct(JSContext*, unsigned int, JS::Value*) at /Users/matthewgertner/Development/mozrelease/browserdebug/dom/bindings/XMLHttpRequestBinding.cpp:1222
#2	0x000000010a1b360a in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) ()
#3	0x000000010a05510b in js::CallJSNativeConstructor(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) ()
#4	0x000000010a04d308 in js::InvokeConstructorKernel(JSContext*, js::CallArgs const&) ()
#5	0x000000010a03d200 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) ()
#6	0x000000010a04d7d2 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) ()

When constructing the XMLHttpRequest, it tries to cast aGlobal to nsIPIDOMWindow and gets null. Not sure if this extra data point is of use to you.

You may remember that I ran into another problem with the global in a subscript: https://bugzilla.mozilla.org/show_bug.cgi?id=746363. In that case I was unable to get a third-party JS library (Jasmine) to work as written due to this issue, and I had to patch it.

IMO it would be worth considering providing loadSubScript with an additional optional parameter specifying the global for the script execution. I don't think we are the only ones using subscripts for "content script" style functionality, and it this case you really want the global to be that of the webpage whose window is the scope for the content script, not that of the chrome window that actually loads the subscript.

Comment 14

[Boris Zbarsky [:bzbarsky]]

If you want content script functionality, shouldn't you be using sandboxes, which are explicitly designed for that?

And yes, if your global is something wacky (is it an XPCOM component global?) then XHR won't work right: it needs a window...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Though it might be worth filing that last as a separate bug with steps to reproduce, because I thought we'd made it work when created via createInstance, so maybe the Constructor() version should behave similarly when there's no window around.

Comment 16

[Matthew Gertner]

(In reply to Boris Zbarsky (:bz) from comment #14)
> If you want content script functionality, shouldn't you be using sandboxes,
> which are explicitly designed for that?

Yes, that might work for us. I will give it a shot tomorrow and report back.

> And yes, if your global is something wacky (is it an XPCOM component
> global?) then XHR won't work right: it needs a window...

As far as I can see, the global is a chrome window. I'm loading a JSM from a script file loaded by a browser.xul overlay, and the loadSubScript call happens in that JSM.

I still think it might be useful to have loadSubScript to take an optional global as a parameter but perhaps that's because I misunderstand the use cases it is intended for. I'll have a better picture once I've tried using sandboxes.

Comment 17

[Matthew Gertner]

(In reply to Boris Zbarsky (:bz) from comment #15)
> Though it might be worth filing that last as a separate bug with steps to
> reproduce, because I thought we'd made it work when created via
> createInstance, so maybe the Constructor() version should behave similarly
> when there's no window around.

"It" means instantiating an XMLHttpRequest when the global is not a window? If I understand correctly I'll go ahead and file a separate bug for that.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> As far as I can see, the global is a chrome window.

Then it would implement nsPIDOMWindow.  So if that bit is faling, something funky is going on.  A separate bug, plus a way to reproduce, would be nice.

Comment 19

[Alex Keybl [:akeybl]]

We'll wait to see if bz's comment 14 resolves this for you all before tracking for release.

Comment 20

[Boris Zbarsky [:bzbarsky]]

I think we need to track this no matter what happens with comment 14.  The behavior here is just wrong.  Too bad no one told us in the 12+ weeks this was in builds before Fx14 shipped.  :(

Comment 21

[Alex Keybl [:akeybl]]

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #20)
> I think we need to track this no matter what happens with comment 14.  The
> behavior here is just wrong.  Too bad no one told us in the 12+ weeks this
> was in builds before Fx14 shipped.  :(

Yeah :\

Do you think we'll take an in-product fix regardless for FF15?

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'm hoping so, yes.  Peter is going to look into what's going on here.

Comment 23

[Matthew Gertner]

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #20)
> I think we need to track this no matter what happens with comment 14.  The
> behavior here is just wrong.  Too bad no one told us in the 12+ weeks this
> was in builds before Fx14 shipped.  :(

We're setting up our CI environment to build/test with upcoming Firefox versions, but unfortunately we haven't completed this yet. Hopefully we'll be able to provide more timely feedback in the future.

Comment 24

[Matthew Gertner]

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #18)
> > As far as I can see, the global is a chrome window.
> 
> Then it would implement nsPIDOMWindow.  So if that bit is faling, something
> funky is going on.  A separate bug, plus a way to reproduce, would be nice.

Actually I guess the global is something different since I'm calling loadSubScript from a JS module. So perhaps this behavior is normal/expected.

What I ended up doing is loading jQuery (which was causing the original problem) using a sandbox. The other scripts I use are still loaded using the subscript loader. Using a sandbox everywhere would be the cleanest solution, but the lack of filename/lineno information is a dealbreaker since we have an implementation of CommonJS require() and load everything dynamically. I tried it but if there's an error anywhere it just tells me "error in Scripting.jsm line [something meaningless]" (where Scripting.jsm is the module where the script loading happens).

I would be very cool to have sandbox support for loading scripts by filename rather than just passing in the script source code, assuming this would allow proper error reporting along the lines of what the subscript loader does.

One other note: I was originally using the content window where my content scripts are being applied (e.g. www.google.com) as the global object for the sandbox. However, this didn't work since jQuery couldn't access the callbacks in the scripts loaded via require() (i.e. with the subscript loader) since it would have needed chrome privileges to do so. So I ended up using a chrome window as the principal and the content window as the sandbox prototype, which seems to work fine.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Matthew, the "no one told us" wasn't about you specifically.  I'm sorry that wasn't clear.

If nothing else it was also about our test suite... ;)

> Actually I guess the global is something different since I'm calling loadSubScript from a
> JS module. So perhaps this behavior is normal/expected.

Ah.  It's still worth filing a bug on this, I think.  We should think about how it should behave, at least.

Comment 26

[Matthew Gertner]

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #25)
> Matthew, the "no one told us" wasn't about you specifically.  I'm sorry that
> wasn't clear.

No worries, it's important for us to catch this kind of stuff earlier and it is something we are working on.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Adding tracking for this, Matthew can you file the bug bz mentions wanting in comment 18?

Comment 28

[Matthew Gertner]

(In reply to Lukas Blakk [:lsblakk] from comment #27)
> Adding tracking for this, Matthew can you file the bug bz mentions wanting

Yeah, I will. I need to find some time to update my test extension so it tries to instantiate XMLHttpRequest in a module. Will try to do so in the next few days.

Comment 29

[Peter Van der Beken [:peterv]]

Attachment: WIP

This isn't quite ready yet, need to keep looking up the interface object in the Xray's global. But at least creating the interface object and defining it on the global is decoupled.

Comment 30

[Matthew Gertner]

(In reply to Lukas Blakk [:lsblakk] from comment #27)
> Adding tracking for this, Matthew can you file the bug bz mentions wanting
> in comment 18?

I haven't been able to reproduce that issue in a reduced test case. I guess it must be something very specific to the behavior of our extension. Hopefully it's just a side-effect of the problem outlined in this bug.

Comment 31

[Peter Van der Beken [:peterv]]

Attachment: v1

Need to document that we store both interface prototype objects and interface objects in the array, in two blocks. Also should rename DOMInterfaceObject.

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

Wontfixing for FF15 which is about to go to final Beta.  Please nominate for uplift to Aurora (or Beta if after 8/28) when ready.

Comment 33

[Alex Keybl [:akeybl]]

Is there any ongoing user impact here? We shipped FF15 without a fix, so although we'd accept an uplift nomination, it's not clear this needs to be tracked any further.

Comment 34

[Boris Zbarsky [:bzbarsky]]

The main user impact is that some extensions might be broken.

Comment 35

[Alex Keybl [:akeybl]]

(In reply to Boris Zbarsky (:bz) from comment #34)
> The main user impact is that some extensions might be broken.

OK - since we haven't heard of add-on breakage in recent updates though, we'll wontfix 16 and untrack 17.

Comment 36

[Peter Van der Beken [:peterv]]

Attachment: v2

- We used to store only the interface prototype object in the global's cache unless there was none and then we stored interface object. We always need the interface object in nsWindowSH::GlobalResolve now (and Interface.prototype is not readonly), so I've made the cache an array of [interface prototypes|interfaces].

- I redid how we do Xrays, we used to store two functions (resolve and enumerate) but I made us store the property lists (we stil have class-specific functions for 'own' properties for the proxies).

- I've started Xraying interface and interface prototype objects. For the ones where we had a JSClass I've converted them to DOMJSClass and set the JSCLASS_IS_DOMJSCLASS flag. This is a little scary, since the flag used to mean "is a DOM object", but that didn't include interface and interface prototype objects. However, I think this is ok since I've set mDOMObjectIsISupports to false and all the mInterfaceChain elements to _ID_Count, so they don't actually "implement" any DOM interface. There's not much you can do with them as a result.

- For constructors that are just functions (where we don't have a specific JSClass) I've switched to a JSFunction with a global JSNative and with reserved slots, one of the slots holds the "real" JSNative and the other slot holds the property lists.

- The Xray code can now call a helper with the property lists (gotten from the DOMJSClass of out of the reserved function slot) to either resolve or enumerate.

- When resolving an interface on an Xray of a window we get an Xray of the interface object off of the window and then define it as a property on the Xray.

- This allows us the remove the weird receiver argument that I added in bug 741267.

This is a big change (though the patch is even bigger because of some renamings), I'm thinking it's not sensible to try to take this on branches. I've tried other ways of fixing this, but this looks like the only option so far.

Comment 37

[Boris Zbarsky [:bzbarsky]]

The one caveat I see for interface and proto objects is that we don't want to consider them to be "platform objects" for purposes of the WebIDL rules, I think...  So we might need a way to test that off the DOMJSClass?

Comment 38

[Boris Zbarsky [:bzbarsky]]

And in particular, should I be able to pass interface or proto objects as instances of callback interfaces?  Yes, I know that's a nutso thing to do. ;)

Comment 39

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Distinguish between DOM objects and DOM interface and interface prototype object with a bool member in DOMJSClass. I renamed a lot of the existing functions, mostly from *DOM* to *DOMObject*. We could use *DOMInstance* or something if you think DOMObject is not clear enough (but there were already some *DOMObject* functions).

Comment 40

[Peter Van der Beken [:peterv]]

Attachment: Changes between v2 to v2.1

Comment 41

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

Comment on attachment 665402 [details] [diff] [review]
v2.1

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

::: dom/bindings/BindingUtils.h
@@ +333,4 @@
>  {
>    MOZ_ASSERT(js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL);
>  
> +  JSObject** ProtoAndIfaceArray = GetProtoAndIfaceArray(obj);

Revert the name change here, please.

Comment 42

[Peter Van der Beken [:peterv]]

Comment on attachment 665402 [details] [diff] [review]
v2.1

There's still some issues :-(.

Comment 43

[Peter Van der Beken [:peterv]]

Attachment: Rename ProtoOrIfaceCache v1

Renaming because we store both interface object and interface prototype object for an interface in the array.

Comment 44

[Peter Van der Beken [:peterv]]

Attachment: Move code v1

Comment 45

[Peter Van der Beken [:peterv]]

Attachment: v3

This adds Xrays for interface and interface prototype objects, looking up a name on an Xray for a window looks up the interface object and wraps it in an Xray instead of looking up the name in the Xrays' compartment.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 674354 [details] [diff] [review]
Rename ProtoOrIfaceCache v1

r=me

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 674356 [details] [diff] [review]
Move code v1

r=me, but how do we plan to handle merging with bug 793267?  Should I hold off on that some more until after you land this?  I guess I should, since this is somewhat higher priority.....

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 674376 [details] [diff] [review]
v3

I think this could use a much more extensive checkin comment that describes what's actually being done.

The extra work we're adding on the fast path in UnwrapObject is sadmaking.  I guess I'll try to keep pushing for us to be able to use the same slot index for proxies and non-proxies so we can nuke most of the slot-related complexity...

>+++ b/dom/bindings/BindingUtils.cpp
>+CreateInterfaceObjects(JSContext* cx, JSObject* global, JSObject* protoProto,
>     js::SetReservedSlot(proto, DOM_PROTO_INSTANCE_CLASS_SLOT,
>                         JS::PrivateValue(const_cast<DOMClass*>(domClass)));

Hmm.  So I dug into this more, and looks like right now the JS engine will call call InstanceClassHasProtoAtDepth on the proto of objects that are JSCLASS_IS_DOMJSCLASS, assuming that proto has not been messed with.

But in our new setup, that flag is set on all sorts of things whose proto will not in fact have a reserved slot with a useful DOMClass in it.

So we need to fix the "isDOM" determination in js::NewObjectWithGivenProto somehow, I think... or something.  Please check with efaust on how best to deal with this end of things?

It's really too bad that TI's type doesn't cover class.  If it did, we wouldn't need this hackery, because TI could just pass the instance JSClass to us directly.  :(

>+XrayResolveNativeProperty(JSContext* cx, JSObject* wrapper,
>+    JSObject* global = js::GetGlobalForObjectCrossCompartment(obj);

There's some copy/paste here for "prototype" and "constructor".  The only difference is the desc->attrs and the id to use.  Could we maybe factor stuff out into a function that takes those two things and obj and desc?

>+XrayResolveNativeProperty(JSContext* cx, JSObject* wrapper, JSObject* obj,
>+  if (type == eInstance) {
>+    // Force the type to be eInterfacePrototype, since we need to walk the
>+    // prototype chain.
>+    type = eInterfacePrototype;

It might be good to more clearly document that the nativePropertyHooks for an eInstance object are exactly the same as for a eInterfacePrototype object for its immediate DOM proto.  Assuming that's true, of course...  If it's not, then nevermind.

Basically, this looks fishy to me but I'm not sure why or how to make it non-fishy.

>+NativeToString(JSContext* cx, JSObject* wrapper, JSObject* obj, const char* pre,
>+    JSAutoCompartment ac(cx, obj);
....
>+      if (JS_CallFunctionValue(cx, obj, toStringDesc.value, 0, nullptr, &toStringValue)) {

But isn't toStringDesc.value in the xray compartment (that is the compartment of "wrapper", not of "obj")?  If so, don't we need to wrap desc once we enter the compartment of "obj"?

>+    str = ConcatJSString(cx, pre, str, post);

So this will still leave the "[object XrayWrapper" bit there even if obj has a stringifier.  Is that actually desirable?  Did we use to do that?

Note that stringifier attributes should still end up resolving to a toString, so I don't think we need any special support for them here.

>+++ b/dom/bindings/BindingUtils.h
>-const size_t kProtoAndIfaceCacheCount =
>-  prototypes::id::_ID_Count + constructors::id::_ID_Count;
>+const size_t kProtoAndIfaceCacheCount = constructors::id::_ID_Count;

I would dearly love a static assert about how prototypes::id::_ID_Count is the same as the first entry of constructors::id.

Maybe we can do that by having CGEnum automatically output a _ID_Start set equal to the value first entry as part of the enum?

>+ * constructor holds the JSNative to back the interface object which should be a
>+ *             Function, unless constructorClass is non-null in which case it is
>+ *             ignored.

We lost the docs about how it can be null if constructorClass is null and we're not supposed to have an interface object.

>+ * constructorCache a pointer to a JSObject pointer where we should cache the
>+ *                  interface object. This must be null if constructorClass or
>+ *                  constructor are and vice versa.

Perhaps:

  This must be null if both constructorClass and constructor are null,
  and non-null otherwise.

>+XrayResolveOwnProperty(JSContext* cx, JSObject* wrapper, JSObject* obj,
>+XrayResolveNativeProperty(JSContext* cx, JSObject* wrapper, JSObject* obj,

Some comments describing these two (and in particular how they differ!) would be very good.

But also, those comments should cover what exactly "obj" is.

>+XrayEnumerateProperties(JSContext* cx, JSObject* wrapper, JSObject* obj,

Again, what's obj?

>+  CONSTRUCTOR_XRAY_EXPANDO_SLOT

Document this one?

>+HasConstructor(JSObject* obj)

Is this debug-only?  If so, perhaps ifdef it?

>+NativeToString(JSContext* cx, JSObject* wrapper, JSObject* obj, const char* pre,
>+               const char* post, JS::Value* v);

Please document what this does and what the args mean.

>+++ b/dom/bindings/Codegen.py
>@@ -5277,54 +5298,53 @@ class CGDescriptor(CGThing):
>+            if descriptor.interface.ctor():
>+                cgThings.append(CGClassConstructHook(descriptor))

Why do we need that check?  CGClassConstructHook already checks for ctor(), no?

>+            if descriptor.hasInstanceInterface:
>+                cgThings.append(CGClassHasInstanceHook(descriptor))
>+                cgThings.append(CGInterfaceObjectDOMJSClass(descriptor, properties))

And likewise here.  CGClassHasInstanceHook and CGInterfaceObjectDOMJSClass both check for hasInstanceInterface....

>+++ b/dom/bindings/DOMJSClass.h
> #define DOM_PROTO_INSTANCE_CLASS_SLOT 0

We need to assert somewhere that DOM_XRAY_EXPANDO_SLOT != DOM_PROTO_INSTANCE_CLASS_SLOT, right?  Because we seem to be depending on that.

> struct NativePropertyHooks

Documentation for what the members of this mean would be nice.

>+++ b/dom/tests/browser/Makefile.in

I mostly skimmed the tests.  Let me know if you want me to review them carefully...

>+++ b/dom/workers/WorkerScope.cpp
>+  if (!FileReaderSyncBinding_workers::GetConstructorObject(aCx, global) ||

Why GetConstructorObject instead of GetProtoObject?  Note that in nsXPConnect::InitClassesWithNewWrappedGlobal you left GetProtoObject callers...  Should one or the other be preferred, and why?

r=me with the above dealt with.

Comment 49

[Peter Van der Beken [:peterv]]

Attachment: v3.1

Now with a different JSClass for interface and interface prototype objects.

> There's some copy/paste here for "prototype" and "constructor".  The only
> difference is the desc->attrs and the id to use.  Could we maybe factor
> stuff out into a function that takes those two things and obj and desc?

Done.

> It might be good to more clearly document that the nativePropertyHooks for
> an eInstance object are exactly the same as for a eInterfacePrototype object
> for its immediate DOM proto.  Assuming that's true, of course...  If it's
> not, then nevermind.

Right, documented.

> But isn't toStringDesc.value in the xray compartment (that is the
> compartment of "wrapper", not of "obj")?  If so, don't we need to wrap desc
> once we enter the compartment of "obj"?

Yup, done.

> So this will still leave the "[object XrayWrapper" bit there even if obj has
> a stringifier.  Is that actually desirable?  Did we use to do that?

Think so, yes. But I've changed it to not do it.

> Note that stringifier attributes should still end up resolving to a
> toString, so I don't think we need any special support for them here.

Removed comment.

> I would dearly love a static assert about how prototypes::id::_ID_Count is
> the same as the first entry of constructors::id.
> 
> Maybe we can do that by having CGEnum automatically output a _ID_Start set
> equal to the value first entry as part of the enum?

Done. I've also explicitly set the first value of constructors::id::ID to prototypes::id::_ID_Count.

> We lost the docs about how it can be null if constructorClass is null and
> we're not supposed to have an interface object.

Readded.

>   This must be null if both constructorClass and constructor are null,
>   and non-null otherwise.

Done.

> Some comments describing these two (and in particular how they differ!)
> would be very good.

Done.

> But also, those comments should cover what exactly "obj" is.

Done.

> Again, what's obj?

Done.

> Document this one?

Done.

> Is this debug-only?  If so, perhaps ifdef it?

Ok, I wasn't sure whether it would be useful in the future, but done.

> Please document what this does and what the args mean.

Done.

> Why do we need that check?  CGClassConstructHook already checks for ctor(),
> no?

Removed.

> And likewise here.  CGClassHasInstanceHook and CGInterfaceObjectDOMJSClass
> both check for hasInstanceInterface....

Removed.

> >+++ b/dom/bindings/DOMJSClass.h
> > #define DOM_PROTO_INSTANCE_CLASS_SLOT 0
> 
> We need to assert somewhere that DOM_XRAY_EXPANDO_SLOT !=
> DOM_PROTO_INSTANCE_CLASS_SLOT, right?  Because we seem to be depending on
> that.

Done.

> Documentation for what the members of this mean would be nice.

Done.

> Why GetConstructorObject instead of GetProtoObject?  Note that in
> nsXPConnect::InitClassesWithNewWrappedGlobal you left GetProtoObject
> callers...  Should one or the other be preferred, and why?

I've changed them all. It doesn't matter in practice, but we're calling these to define the property on the global that refers to the interface object so GetConstructorObject seems more logical.

Comment 50

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/76ffe1c8d453
https://hg.mozilla.org/mozilla-central/rev/7869880fd020

Comment 51

[Peter Van der Beken [:peterv]]

Not everything was landed yet, sorry.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee6b1acd5ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/acc131324855

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ee6b1acd5ea
https://hg.mozilla.org/mozilla-central/rev/acc131324855