816088 - webIDL bindings try to extract nsISupports from the global object in static properties in workers.

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Andrea Marchesini [:baku]]

... but the global object in workers is not an nsISupports, so we throw.

Comment 1

[Masatoshi Kimura [:emk]]

You need to inherit from DOMBindingBase, I think.
See existing classes, bug 793025 and bug 795542 about WebIDL bindings objects in workers.
<https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings> really should have a document about workers...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes, it should.  Sadly, it'll need to be an almost entirely separate document or something; a lot of things are different in workers.  :(  That said, I'd be happy if someone writes it up; it doesn't have to be me, since I don't particularly know the workers bindings better than anyone else....

Comment 3

[Andrea Marchesini [:baku]]

I do inherit from DOMBindingBase through EventTarget: https://bugzilla.mozilla.org/attachment.cgi?id=686105 but this doesn't seem enough. Can you give me a feedback about this patch?

Comment 4

[Peter Van der Beken [:peterv]]

This bug is about a problem in the codegen for static properties, totally unrelated to comment 1.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

This changes the type of the global objects to mozilla::dom::GlobalObject (which stores an nsISupports) and mozilla::dom::GlobalObjectForWorker (which stores a JSContext and a JSObject). Not sure if these "public" types should have their own header. Note also that argument order is now a bit messy because the global object always comes first, even if we pass in a JSContext. That makes it easier to detect the right signature for bug 815404. But if there's no global then JSContext always comes first. I didn't try to eliminate the cx argument for workers if we already pass it in the global object.

Comment 6

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

(In reply to Peter Van der Beken [:peterv] from comment #5)
> Created attachment 692309 [details] [diff] [review]
> v1
> 
> Not sure if these "public" types should have their own header.

I think so, yes. We've been dumping rather a lot in BindingUtils.h.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Need to update docs at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#C.2B.2B_reflections_of_WebIDL_constructors at least.  And we should document static operations/attributes, actually...

As far as headers, I think we should move this and perhaps Optional and such out of BindingUtils into a separate header or headers, so you can get those types without having to pull in all the xpconnect goop the various wrapping functions pull in.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 692309 [details] [diff] [review]
v1

>+++ b/dom/bindings/BindingUtils.cpp
>+GetGlobalObjectForThis(JSContext* aCx, JS::Value* aVp)
>+  if (thisObj && js::IsWrapper(thisObj)) {

But if it's not a wrapper, we just return null?  This seems wrong.

Note also bug 821760; we'll need to merge depending on which of these lands first.

>+  return nullptr;

This should be "return JS_GetGlobalForObject(aCx, thisObj);", I would think.  At least if thisObj is nonnull.

>+UnwrapGlobalObject(JSContext *aCx, JSObject* aObj, nsISupports** aResult,

Should this assert that aObj is a global if it's not null?  I would prefer that.

>+++ b/dom/bindings/BindingUtils.h

The code duplication between GlobalObjectForConstructor and GlobalObjectForStatic could be avoided by pushing those bits up into GlobalObject, no?  Is there a reason not to do that?

>+++ b/dom/bindings/Codegen.py
>+        callGenerator = CGMethodCall([], nativeName, True,

This patch removes the only consumers of argsPre on CGMethodCall.  Can we just eliminate that argument completely?  I think we may also be able to get rid of the argsPre argument on CGPerSignatureCall too, for the same reason: all callers pass [].

>@@ -3580,42 +3556,62 @@ class CGPerSignatureCall(CGThing):
>+if (getGlobal.Failed()) {
>+  return false;

There's been no exception thrown on the JSContext, so this will uncatchably stop script execution.  We should be throwing an exception somewhere....

>@@ -6993,21 +6983,23 @@ class CGNativeMember(ClassMethod):

You need to reverse the order of the global and cx arguments here.  Please throw a static operation that involves a cx into TestCode/ExampleGen, so this will be exercised?

r- because I want to see the updates to those helper structs...

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 701277 [details] [diff] [review]
v2

>+class GlobalObject
>+class WorkerGlobalObject

These are stack-only, right?  Please flag them as such.

Perhaps document that the cx in WorkerGlobalObject need not be in the compartment of what Get() returns (and in fact will generally be in the caller's compartment)?

> JSObject* obj = JS_GetGlobalForObject(cx, JSVAL_TO_OBJECT(JS_CALLEE(cx, vp)));

Does that still need to call GetGlobalForObject?  Or could it just use the JS_CALLEE directly?  Either way, I guess.


> // We have to be careful to leave "obj" in its existing compartment

This whole comment no longer seems to have any bearing on anything.  Can we just remove it?

r=me with those nits.

Comment 12

[Vendelin Ruzicka [:vendo]]

Attachment: Windows using fix

Hi, small fix contains "using" commands added in attachment.

Comment 13

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5075690572a9

Comment 14

[Ed Morley [:emorley]]

Push backed out for OS X startup failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f3c145bd1dd2

https://hg.mozilla.org/integration/mozilla-inbound/rev/18b2fd105563

Comment 15

[Peter Van der Beken [:peterv]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #14)
> Push backed out for OS X startup failures:

Which weren't actually related to this patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/260de4b5771d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/260de4b5771d

Comment 17

[Boris Zbarsky [:bzbarsky]]

Documented: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings$compare?to=362923&from=358271