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

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
2 months ago

People

(Reporter: matthew.gertner, Assigned: peterv)

Tracking

({regression})

unspecified
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14 affected, firefox15+ wontfix, firefox16+ wontfix, firefox17- affected)

Details

Attachments

(4 attachments, 6 obsolete attachments)

Reporter

Description

7 years ago
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.
Reporter

Updated

7 years ago
Attachment #646589 - Attachment mime type: text/plain → application/x-xpinstall
Reporter

Updated

7 years ago
Blocks: 740069
Reporter

Comment 1

7 years ago
Sorry, it's been a long day. The value of window.XMLHttpRequest is undefined, not null.
I get things like:

--- SUCCESS: [object Object]

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

Comment 3

7 years ago
Current means trunk?
But in a current nightly I do have the problem, apparently.  Weird.
Yes, current means trunk.
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.
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
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.
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....
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...
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
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.... ;)
Keywords: regression
Summary: Content window does not have an XMLHttpRequest property → Content window does not have an XMLHttpRequest property when accessed via an Xray wrapper in a subscript
Reporter

Comment 13

7 years ago
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.
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...
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.
Reporter

Comment 16

7 years ago
(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.
Reporter

Comment 17

7 years ago
(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.
> 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.
We'll wait to see if bz's comment 14 resolves this for you all before tracking for release.
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.  :(
(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?
Assignee

Updated

7 years ago
Assignee: nobody → peterv
OS: Mac OS X → All
Hardware: x86 → All
I'm hoping so, yes.  Peter is going to look into what's going on here.
Reporter

Comment 23

7 years ago
(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.
Reporter

Comment 24

7 years ago
(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.
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.
Reporter

Comment 26

7 years ago
(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.
Adding tracking for this, Matthew can you file the bug bz mentions wanting in comment 18?
Reporter

Comment 28

7 years ago
(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.
Assignee

Comment 29

7 years ago
Posted patch WIP (obsolete) — Splinter Review
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.
Reporter

Comment 30

7 years ago
(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.
Assignee

Comment 31

7 years ago
Posted patch v1 (obsolete) — Splinter Review
Need to document that we store both interface prototype objects and interface objects in the array, in two blocks. Also should rename DOMInterfaceObject.
Attachment #650531 - Attachment is obsolete: true
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.
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.
The main user impact is that some extensions might be broken.
(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.
Assignee

Comment 36

7 years ago
Posted patch v2 (obsolete) — Splinter Review
- 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.
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?
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. ;)
Assignee

Comment 39

7 years ago
Posted patch v2.1 (obsolete) — Splinter Review
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).
Attachment #652401 - Attachment is obsolete: true
Attachment #663397 - Attachment is obsolete: true
Attachment #665402 - Flags: review?(bzbarsky)
Assignee

Comment 40

7 years ago
Posted patch Changes between v2 to v2.1 (obsolete) — Splinter Review
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.
Assignee

Comment 42

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

There's still some issues :-(.
Attachment #665402 - Flags: review?(bzbarsky)
Assignee

Updated

7 years ago
Depends on: 804558
Assignee

Comment 43

7 years ago
Renaming because we store both interface object and interface prototype object for an interface in the array.
Attachment #674354 - Flags: review?(bzbarsky)
Assignee

Comment 44

7 years ago
Posted patch Move code v1Splinter Review
Attachment #674356 - Flags: review?(bzbarsky)
Assignee

Updated

7 years ago
Attachment #674354 - Attachment description: v1 → Rename ProtoOrIfaceCache v1
Assignee

Comment 45

7 years ago
Posted patch v3 (obsolete) — Splinter Review
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.
Attachment #665402 - Attachment is obsolete: true
Attachment #665403 - Attachment is obsolete: true
Attachment #674376 - Flags: review?(bzbarsky)
Comment on attachment 674354 [details] [diff] [review]
Rename ProtoOrIfaceCache v1

r=me
Attachment #674354 - Flags: review?(bzbarsky) → review+
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.....
Attachment #674356 - Flags: review?(bzbarsky) → review+
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.
Attachment #674376 - Flags: review?(bzbarsky) → review+
No longer blocks: 793267
Assignee

Comment 49

7 years ago
Posted patch v3.1Splinter Review
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.
Attachment #674376 - Attachment is obsolete: true
Attachment #676272 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/76ffe1c8d453
https://hg.mozilla.org/mozilla-central/rev/7869880fd020
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee

Comment 51

7 years ago
Not everything was landed yet, sorry.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee6b1acd5ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/acc131324855
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/4ee6b1acd5ea
https://hg.mozilla.org/mozilla-central/rev/acc131324855
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 807548
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.