1073124 - Improve JSM WebIDL ergonomics and Implement [Exposed=System]

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Our current mechanism for using WebIDL classes in JSMs and JS XPCOM components kinda sucks for two reasons:

1) Callers need to do Cu.importGlobalProperties['ClassName'] before using it.
2) Implementors need to go manually add a call to FooBinding::GetConstructorObject and associated machinery in xpc::GlobalProperties.

(1) is a policy decision which I'm happy to reverse for BackstagePass globals at this point.

(2) Can be fixed by implementing [Exposed=System].

I'm wary in general about making it too easy to expose classes to non-Window scopes, because DOM implementations often make assumptions about the scope being a DOM scope. But I'm ok with it now that we have a strict review policy for dom/webidl, so long as we make it crystal clear to all DOM peers that [Exposed=System] requires (a) an audit of the code to make sure it doesn't depend on Windows and Documents and (b) simple xpcshell test coverage.

We'd basically need to generated something like RegisterWorkerBindings.cpp called RegisterSystemBindings.cpp or somesuch. Unlike workers, I think we should do lazy resolution here, otherwise each additional constructor's memory footprint will be multiplied by ~200.

Once we do that, we just need to invoke it from BackstagePass::NewResolve and we should be good to go.

Comment 1

[Bobby Holley (:bholley)]

Kyle is going to find someone to own this.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

- "System" is a bit vague. What does that mean? TabChildGlobal? JS components?
- what code does (a) talk about?


Other than that, sound good.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #2)
> - "System" is a bit vague. What does that mean? TabChildGlobal? JS
> components?

I was thinking JSMs, JS XPCOM components and (now that you mention it) TabChildGlobal. I'm open to better naming suggestions.

> - what code does (a) talk about?

Not sure I understand your question. I'm saying that we want to make sure that the binding implementation never assumes that the global is an nsGlobalWindow before we expose it to non-DOM globals.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Bobby Holley (:bholley) from comment #3)
> Not sure I understand your question. I'm saying that we want to make sure
> that the binding implementation never assumes that the global is an
> nsGlobalWindow before we expose it to non-DOM globals.
And you answered here. You're talking about the implementation of some webidl interface.
Note, so far .webidl reviews haven't required DOM peer to review all of the implementation, but
it has been there to make sure we expose only the right stuff to the web etc.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Note, so far .webidl reviews haven't required DOM peer to review all of the
> implementation, but
> it has been there to make sure we expose only the right stuff to the web etc.

And they still won't for the most part, unless somebody is exposing an interface to non-Window scopes. It should be a pretty simple 20-second grep, and tbh I care more about the test coverage anyway.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Bug 1048698 was suggesting calling this Exposed="XPConnect" or Exposed="NonDOM", right?  Is that bug just a duplicate of this one?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Bug 1048698 was suggesting calling this Exposed="XPConnect" or
> Exposed="NonDOM", right?  Is that bug just a duplicate of this one?

Yes. Kyle asked me to file a bug and I couldn't find the other one.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Hack the "System" global name into the Web IDL parser, since we have no actual Web IDL interface for system globals

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Define Exposed=System things in BackstagePass::NewResolve as needed

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make PromiseDebugging [Exposed=System]

Comment 12

[Boris Zbarsky [:bzbarsky]]

https://tbpl.mozilla.org/?tree=Try&rev=5e46c43a7b4e

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8505027 [details] [diff] [review]
part 2.  Define Exposed=System things in BackstagePass::NewResolve as needed

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

I keep glancing at this patch and seeing the Codegen.py changes and thinking "ugh". If they need me to really look at them, please upload the generated code, or let me know that I should look at the Codegen more carefully. Otherwise, I trust you. Sorry for holding this up so long before deciding that. r=me

::: dom/bindings/Codegen.py
@@ +14454,5 @@
>  
>      @staticmethod
> +    def ResolveSystemBinding(config):
> +
> +        # TODO - Generate the methods we want

TODO?

::: js/xpconnect/src/XPCRuntimeService.cpp
@@ +71,1 @@
>      *_retval = ResolveWorkerClasses(cx, obj, id, &objp);

Can you file a followup to remove this part in favor of Exposed=System?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: ResolveSystemBinding.cpp

> If they need me to really look at them, please upload the generated code

Here you go.  I hacked a few interfaces (ImageData, WebGLBuffer) to be Exposed=System so you can see what the overall code flow looks like.

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8506944 [details]
ResolveSystemBinding.cpp

I'll trust you on making sure that the ConstructorEnabled calls are happening in the right places. Looks good in general. Thanks!

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Can you file a followup to remove this part in favor of Exposed=System?

Ah, this caught an actual bug.  I wasn't doing anything for this stuff in BackstagePass::Enumerate.

I added the same setup as ResolveWorkerClasses: passing JSID_VOID as the handle will make it match everything, and added a call like that to BackstagePass::Enumerate.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #16)
> > Can you file a followup to remove this part in favor of Exposed=System?
> 
> Ah, this caught an actual bug.  I wasn't doing anything for this stuff in
> BackstagePass::Enumerate.

Doh, sorry for missing that in review.

Comment 18

[Boris Zbarsky [:bzbarsky]]

In any case, the TODO is removed and bug 1084439 is filed.

Comment 19

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/288827a46e4d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/06450cede5ec
   https://hg.mozilla.org/integration/mozilla-inbound/rev/e6c58647bf2b

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/288827a46e4d
https://hg.mozilla.org/mozilla-central/rev/06450cede5ec
https://hg.mozilla.org/mozilla-central/rev/e6c58647bf2b

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Anyone want to document this in MDN?

Comment 23

[Boris Zbarsky [:bzbarsky]]

https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Exposed