Closed Bug 1081241 Opened 10 years ago Closed 10 years ago

Exposed=Worker should prevent interface from showing up in RegisterBindings

Categories

(Core :: DOM: Core & HTML, defect)

34 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: smaug, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Looks like Nikhil actually fixed this in bug 1048699.  Specifically, this part in CGConstructorEnabled:

        if not iface.isExposedInWindow():
            exposedInWindowCheck = dedent(
                """
                if (NS_IsMainThread()) {
                  return false;
                }
                """)
            body.append(CGGeneric(exposedInWindowCheck))

and the fact that isExposedConditionally is true if self.hasThreadChecks() and that's true if (not self.workers and not self.interface.isExposedInWindow()).

So a non-worker descriptor that's not exposed in window will end up with that NS_IsMainThread check.

Seems better to not register it at all, though, so I'm going to morph this bug to do that.
Summary: Exposed=Worker should prevent interface to show up in the Window scope → Exposed=Worker should prevent interface from showing up in RegisterBindings
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8523477 [details] [diff] [review]
Things that aren't exposed in Window shouldn't show up in RegisterBindings at all


>+        return (self.isExposedConditionally() and
>+                (not self.interface.isExposedInWindow() or
>                  self.interface.isExposedOnlyInSomeWorkers()))
is a bit hard to read, especially since self.isExposedConditionally() has magical meaning
(self.interface.isExposedConditionally() + self.interface.isExposedOnlyInSomeWorkers() meaning).
So why not
return (self.interface.isExposedConditionally() and not self.interface.isExposedInWindow()) or
       self.interface.isExposedOnlyInSomeWorkers()



>     def isExposedOnlyInSomeWorkers(self):
>-        assert self.isExposedInAnyWorker()
>+        if not self.isExposedInAnyWorker():
>+            return False
>         workerScopes = self.parentScope.globalNameMapping["Worker"]
Not really about this bug but
I don't understand why self.parentScope.globalNameMapping["Worker"] does what it seems to do, but apparently
"Worker" term is used in rather broad way here.
Also, is isExposedOnlyInSomeWorkers supposed to return true only if isExposedInWindow returns false?
Or perhaps 'Only' refers to a set of possible Worker-type of globals, not all Globals
The naming really could be a bit better.
Attachment #8523477 - Flags: review?(bugs) → review+
>So why not

OK.  I was basically modeling it as "we generate the IsConstructorEnabled method, and whatever conditions cause that method to use NS_IsMainThread".  But your formulation is equivalent.

> I don't understand why self.parentScope.globalNameMapping["Worker"] does what it seems

Because the IDL for workers looks like this:

  [Global=(Worker,DedicatedWorker),
   Exposed=DedicatedWorker]
  interface DedicatedWorkerGlobalScope : WorkerGlobalScope {

and

  [Global=(Worker,SharedWorker),
   Exposed=SharedWorker]
  interface SharedWorkerGlobalScope : WorkerGlobalScope {

and

  [Global=(Worker,ServiceWorker),
   Exposed=ServiceWorker]
  interface ServiceWorkerGlobalScope : WorkerGlobalScope {

In other words, all the worker global interfaces explicitly say they are Worker in addition to whatever else they are, so you can use [Exposed=Worker] to expose things in all workers.

> Also, is isExposedOnlyInSomeWorkers supposed to return true only if isExposedInWindow
> returns false?

No.  And it doesn't.  What this is doing is taking the set of all worker scopes, subtracting out the scopes we're exposed in, and returning whether the result is the empty set.  Since Window is not in the set of all worker scopes, it doesn't matter whether we're exposed in Window or not.

The reason we need the isExposedInAnyWorker check is to not claim isExposedOnlyInSomeWorkers when we're not exposed in workers at all.

> Or perhaps 'Only' refers to a set of possible Worker-type of globals

Yes.

> The naming really could be a bit better.

I could call this isNotExposedInAllWorkers, but that would probably require going back to requiring all callers to check isExposedInAnyWorker...  If you have good naming ideas here, I'm all ears.
isExposedInSomeButNotAllWorkers?
Flags: needinfo?(bugs)
That is quite clear (+ a comment somewhere that it doesn't say anything about exposing the interface in Window context)
Flags: needinfo?(bugs)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8646f1c6339d with the better name and a comment.
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/8646f1c6339d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.