1048699 - Make Exposed=SpecificWorkerType work automagically

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we only put things that are Exposed=Workers into RegisterWorkerBindings.

We should instead consider putting in everything that's defined in any worker (e.g. Exposed=SharedWorker) but auto-generating IsEnabled functions based on the worker type.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Maybe not a true blocker, but link this up to the meta bug so we don't lose it.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Make Exposed=SpecificWorkerType work automatically

Generates (IsFooWorker()) for every binding before the call to its ConstructorEnabled().

Comment 3

[Boris Zbarsky [:bzbarsky]]

So I was thinking through this and I may have led you astray.

With this setup, the descriptor also needs to be flagged as worker-only in Bindings.conf to avoid it getting set up on Window globals, right?

If we were adding conditions to ConstructorEnabled instead, then we wouldn't need to touch Bindings.conf: the ConstructorEnabled check would handle things.

Thoughts?  

If we decide to go this route, we could change isExposedConditionally() to check for cases like "non-worker descriptor with interface not exposed on Window" or "descriptor exposed on some, but not all, workers" and return true if so, and change ConstructorEnabled to consider exposure sets.

Apart from that, I think we should add an API on IDLInterface to ask it whether it's exposed in any workers instead of assuming that _exposureGlobalNames that contain "Worker" mean it is.  Specifically, the interface could look at self.parentScope.globalNameMapping["Worker"] and intersect that with self.exposureSet.  If the intersection is non-empty, the interface is exposed in _some_ worker and we should put it in RegisterWorkerBindings.

In any case, isExposedInAllWorkers should be renamed to isExposedInAnyWorker, right?

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> So I was thinking through this and I may have led you astray.
> 
> With this setup, the descriptor also needs to be flagged as worker-only in
> Bindings.conf to avoid it getting set up on Window globals, right?

Correct.
> 
> If we were adding conditions to ConstructorEnabled instead, then we wouldn't
> need to touch Bindings.conf: the ConstructorEnabled check would handle
> things.
> 
> Thoughts?

Yea I'll try this approach.
> 
> If we decide to go this route, we could change isExposedConditionally() to
> check for cases like "non-worker descriptor with interface not exposed on
> Window" or "descriptor exposed on some, but not all, workers" and return
> true if so, and change ConstructorEnabled to consider exposure sets.

Is this just a note or something that affects this bug?
> 
> Apart from that, I think we should add an API on IDLInterface to ask it
> whether it's exposed in any workers instead of assuming that
> _exposureGlobalNames that contain "Worker" mean it is.  Specifically, the
> interface could look at self.parentScope.globalNameMapping["Worker"] and
> intersect that with self.exposureSet.  If the intersection is non-empty, the
> interface is exposed in _some_ worker and we should put it in
> RegisterWorkerBindings.
> 
> In any case, isExposedInAllWorkers should be renamed to
> isExposedInAnyWorker, right?

Right.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Make Exposed=SpecificWorkerType work automatically

I'm confused about how to check for the various globals inside the ConstructorEnabled() body. Do I just unwrap the global passed to the function and check if it is the type required?

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #5)
> Created attachment 8479513 [details] [diff] [review]
> Make Exposed=SpecificWorkerType work automatically
> 
> I'm confused about how to check for the various globals inside the
> ConstructorEnabled() body. Do I just unwrap the global passed to the
> function and check if it is the type required?

Especially, how do I check for window? afaik on the main thread the global can also be a sandbox.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Is this just a note or something that affects this bug?

It was an implementation suggestion for this bug.

> afaik on the main thread the global can also be a sandbox.

Good catch.  So what I think we should do is the following, for now:

1)  Check for "Window" by doing NS_IsMainThread().
2)  Check for workers as needed by comparing the js::GetObjectClass()->name of the global
    to the things in the interface.exposureSet.

and I'll think a bit more about how to handle various non-Window mainthread globals in bug 1048698.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8479513 [details] [diff] [review]
Make Exposed=SpecificWorkerType work automatically

This is missing the codegen bits, but the part that's here is good.  ;)

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Make Exposed=SpecificWorkerType work automatically

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Relevant try run
https://tbpl.mozilla.org/?tree=Try&rev=7ac9f4ab4b22

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8480164 [details] [diff] [review]
Make Exposed=SpecificWorkerType work automatically

>+          exposedInWindowCheck = fill("""

You probably want dedent(), not fill(), and put the """ on the next line (which will also auto-indent better to make it clearer that the "if" is not Python code).  It should end up looking something like this:

          exposedInWindowCheck = dedent(
              """
              if (NS_IsMainThread()) {
                return false;
              }
              """)

>+            workerGlobals = filter(lambda x: x.find('WorkerGlobalScope') > 0, iface.exposureSet)

I would really prefer it if this were something you asked the IDLInterface, because the IDLInterface can directly tell you this information.  Specifically, this is just the set whose length isExposedInAnyWorker is checking in your implementation, no?

>+            workerCondition = CGList((CGGeneric('strcmp(name, "%s")'% workerGlobal) for workerGlobal in workerGlobals), " && ")

You want sorted(workerGlobals) so codegen is deterministic.  And maybe a linebreak before "for" to not end up with an overlong line.

>+            exposedInWorkerCheck = fill("""

Again, """ on next line.

>+        conditionsWrapper = CGGeneric("return true;\n")
>+        if len(conditions):

How about putting that set to CGGeneric("return true;\n") in the else clause of the if?  I think that would make things clearer.


>+        return CGList([CGGeneric(exposedInWindowCheck or ''),
>+                       CGGeneric(exposedInWorkerCheck or ''),
>+                       conditionsWrapper], "\n").define()

This will produce a bunch of leading newlines when there are no exposed checks, right?  Seems like what we really want here is to create a CGList then add those CGGenerics to it conditionally.

>                     + condition)
>+
>             conditions.append(condition)

That newline addition looks odd to me.  Why do it?

>+        hasWorkerStuff = hasWorkerStuff or any(d.interface.isExposedInAnyWorker() for d in descriptors)
>         bindingHeaders["WorkerPrivate.h"] = hasWorkerStuff
>         bindingHeaders["nsThreadUtils.h"] = hasWorkerStuff

I think this will overinclude.  In particular, it will include for cases like ImageData that are exposed everywhere, right?

This should use a condition similar to the one isExposedConditionally() uses for worker bits.  Possibly factor that out into a separate method on Descriptor called from both places?

The rest looks lovely.  r=me with the above bits fixed

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e5931282ddd

Comment 13

[Boris Zbarsky [:bzbarsky]]

Nikhil, your editor is mis-indenting Python code.  Specifically:

1)  The indentation inside dedent() for the string should have been 4-space, not 2-space. 
    Same for fill().
2)  The "for workerGlobal" bit is mis-indented: it should line up with the CGGeneric on the
    previous line.
3)  The " &&n\n" string in the definition of conditionsWrapper is totally mis-indented.
    It should line up with the '(' before CGGeneric.

Also on that previous line there should be a space between ')' and '%'.

You never answered the "That newline addition looks odd to me.  Why do it?" part above.  I think that newline makes the code pretty confusing.

The landed patch I think _underincludes_ in that it will fail to include nsThreadUtils.h in some cases when it should.

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 2 - Indentation and proper include

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8480829 [details] [diff] [review]
Patch 2 - Indentation and proper include

>+        def descriptorHasThreadChecks(desc):

This is duplicating an identical check that's part of isConditionallyExposed.  Could we make this a method on Descriptor and call it from both places?

r=me with that.  Thanks!

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58c44d7982af

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e5931282ddd
https://hg.mozilla.org/mozilla-central/rev/58c44d7982af