Closed Bug 816088 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: baku, Assigned: peterv)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

... but the global object in workers is not an nsISupports, so we throw.
Assignee: nobody → peterv
Blocks: 813253
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...
Keywords: dev-doc-needed
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....
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?
This bug is about a problem in the codegen for static properties, totally unrelated to comment 1.
Summary: webIDL bindings try to extract nsISupports from the global object in workers. → webIDL bindings try to extract nsISupports from the global object in static properties in workers.
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #692309 - Flags: review?(bzbarsky)
(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.
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 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...
Attachment #692309 - Flags: review?(bzbarsky) → review-
Attached patch v2Splinter Review
Attachment #692309 - Attachment is obsolete: true
Attachment #701277 - Flags: review?(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.
Attachment #701277 - Flags: review?(bzbarsky) → review+
Duplicate of this bug: 834141
Blocks: SyncIDB
Hi, small fix contains "using" commands added in attachment.
(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
https://hg.mozilla.org/mozilla-central/rev/260de4b5771d
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.