Make Exposed=SpecificWorkerType work automagically

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: bzbarsky, Assigned: nsm)

Tracking

(Blocks 1 bug)

unspecified
mozilla34
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Flags: needinfo?(bzbarsky)
Depends on: 1017988
Maybe not a true blocker, but link this up to the meta bug so we don't lose it.
Generates (IsFooWorker()) for every binding before the call to its ConstructorEnabled().
Attachment #8479327 - Flags: review?(bzbarsky)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
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?
Flags: needinfo?(bzbarsky) → needinfo?(nsm.nikhil)
(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.
Flags: needinfo?(nsm.nikhil)
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?
Attachment #8479513 - Attachment is patch: true
Attachment #8479513 - Attachment mime type: application/octet-stream → text/plain
Attachment #8479513 - Flags: feedback?(bzbarsky)
Attachment #8479327 - Attachment is obsolete: true
Attachment #8479327 - Flags: review?(bzbarsky)
(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.
> 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 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.  ;)
Attachment #8479513 - Flags: feedback?(bzbarsky) → feedback+
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
Attachment #8480164 - Flags: review?(bzbarsky) → review+
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.
Flags: needinfo?(nsm.nikhil)
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!
Attachment #8480829 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/8e5931282ddd
https://hg.mozilla.org/mozilla-central/rev/58c44d7982af
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: needinfo?(nsm.nikhil)
You need to log in before you can comment on or make changes to this bug.