958667 - Add a WebIDL annotation for the scope a method/attribute/interface is exposed to

Status: RESOLVED FIXED

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

Description

[(no longer active)]

It would be nice if we could express the scope where an interface/method/attribute is visible in through a WebIDL annotation.  Strawman proposal: [Scope=window,packaged-app,certified-app].

Comment 1

[Boris Zbarsky [:bzbarsky]]

Isn't "window" the default, basically?  If not, what does it mean?

I assume "packaged-app" implies availability in certified apps?

Also, how do you feel about calling it [AvailableIn]?

Do we need to add some sort of built-in hook for permissions checks as well?  That can get a bit complicated, since in general we need to deal with various boolean operations on permissions...

Comment 2

[(no longer active)]

(In reply to comment #1)
> Isn't "window" the default, basically?  If not, what does it mean?

It is...  And speaking of which, we probably don't need that.  There is very little use in an API that is hidden from window.  ;-)

> I assume "packaged-app" implies availability in certified apps?

Correct.

> Also, how do you feel about calling it [AvailableIn]?

That you're clearly better at naming things than I am.  :-)

> Do we need to add some sort of built-in hook for permissions checks as well? 
> That can get a bit complicated, since in general we need to deal with various
> boolean operations on permissions...

Yeah, I thought about that, but I don't have a good proposal of what that should look like yet, for this reason...  Anyways, we can do this for now while we figure out what to do with the permissions.  I can triage the existing usages of permissions to see what kinds of constructs we would need to express to at least be able to move the existing checks into a WebIDL annotation.

Comment 3

[(no longer active)]

Oh, I also think that this condition should be anded with whatever other existing conditions such as [Pref] and [Func] exist on the names.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, I was thinking I should just go ahead and revamp the handling of all these condition bits so they stop being mutually exclusive and get anded together.  If I do it right, it should be pretty simple.

So I need a good way to detect apps and certified apps.  On main thread, if I have a window, I figure I can just get its principal and GetAppStatus() on that.

For workers, I assume I need to stash the app status of the workerprivate, just like we stash whether it's system

Comment 5

[(no longer active)]

(In reply to comment #4)
> Yeah, I was thinking I should just go ahead and revamp the handling of all
> these condition bits so they stop being mutually exclusive and get anded
> together.  If I do it right, it should be pretty simple.

\o/

> So I need a good way to detect apps and certified apps.  On main thread, if I
> have a window, I figure I can just get its principal and GetAppStatus() on
> that.

Yeah I believe that's the "canonical" way of doing that.

> For workers, I assume I need to stash the app status of the workerprivate, just
> like we stash whether it's system

That sounds sane...

Comment 6

[Boris Zbarsky [:bzbarsky]]

So doing this for interfaces is not too bad.

Doing this for individual properties is a bit of a pain because we currently have 3 totally separate mechanisms for handling conditional properties (separate non-chrome/chrome arrays, boolean pref flag in Prefable, and function pointers to the function specified via Func) none of which use a codegenned function.  But the desired end state is clearly to be able to use a codegenned function here.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, but in practice there is no use case for using Func with the annotations here, since you can just call this stuff just as well from inside your custom function if you're doing one anyway.  So maybe I can just assume for now that Func won't be used together with AvailableIn; that will help.

Comment 8

[Boris Zbarsky [:bzbarsky]]

As in, I'll add an easy way for custom functions to check these conditions.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So more precisely, I'll allow both Func and AvailableIn on interface objects, just not on interface members...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Though actually, nevermind.  I can just add another pointer to Prefable for this special case.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make it possible to use multiple methods of disabling the visibility of a constructor at once, disabling it if any of them says it should be disabled.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Store in workers whether they come from apps, and if so whether the app is certified.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add helper methods for testing whether things should be exposed in apps or certified apps.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Hook up an AvailableIn extended attribute for interfaces.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Hook up AvailableIn for interface members.

This is the one patch I'm not too happy with, but I can't see a better simple way to do this.  Also, happy to bikeshed the naming more (e.g. we could have two separate extended attributes [PackagedAppOnly] and [CertifiedAppOnly] or something, instead of [AvailableIn=PackagedApps] and [AvailableIn=CertifiedApps].  And also also, I have no idea how to test whether the app-related annotations actually work.  Especially the worker part...  Even manual testing would be good; right now there's just been none.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add helper methods for testing whether things should be exposed in apps or certified apps.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Hook up an AvailableIn extended attribute for interfaces.

Comment 18

[Robert Kaiser]

(In reply to Boris Zbarsky [:bz] from comment #15)
> [AvailableIn=PackagedApps] and [AvailableIn=CertifiedApps]

Should this mean "privileged" instead of "packaged"? I thought that packaged and hosted apps should have access to exactly the same APIs and the differentiator was only if they're privileged or certified (which in the wild only packaged apps can be due to signing requirements).

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Should this mean "privileged" instead of "packaged"?

I don't know.  What I implemented based on comment 0 uses "status != APP_STATUS_NOT_INSTALLED", so includes installed, privileged, and certified apps.  It would be pretty simple to restrict to only privileged and certified, but then I agree we should rename the annotation to PrivilegedApps.

Comment 20

[(no longer active)]

Yeah I think privileged is the correct name.

Comment 21

[(no longer active)]

Erm, I mean, when I said "packaged" in comment 0, I meant "privileged".  My fingers apologize.  :-)

Comment 22

[Boris Zbarsky [:bzbarsky]]

OK.  So we want to name it Privileged and also have the app type check check for "privileged or certified" for that setting, right?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Store in workers whether they come from privileged or certified apps.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add helper methods for testing whether things should be exposed in privilegd or certified apps.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add helper methods for testing whether things should be exposed in privilegd or certified apps.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Hook up an AvailableIn extended attribute for interfaces.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Hook up AvailableIn for interface members.

Comment 28

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #22)
> OK.  So we want to name it Privileged and also have the app type check check
> for "privileged or certified" for that setting, right?

Yes.  Basically we're operating under this assumption that we never want an API which is only exposed to privileged apps but not certified apps.

Comment 29

[Peter Van der Beken [:peterv]]

Comment on attachment 8358745 [details] [diff] [review]
part 1.  Make it possible to use multiple methods of disabling the visibility of a constructor at once, disabling it if any of them says it should be disabled.

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

::: dom/bindings/Codegen.py
@@ +2096,5 @@
> +        func = iface.getExtendedAttribute("Func")
> +        if func:
> +            assert isinstance(func, list) and len(func) == 1
> +            conditions.append("%s(aCx, aObj)" % func[0])
> +        if iface.getExtendedAttribute("PrefControlled"):

I wish we could replace these with Func instead :-/.

@@ +2102,5 @@
> +        # We should really have some conditions
> +        assert len(conditions)
> +        body = CGList((CGIfWrapper(CGGeneric("return false;"), "!%s" % cond) for
> +                       cond in conditions), "\n\n")
> +        body.append(CGGeneric("return true;"))

Or you could do something like:

    body.append(CGWrapper(CGList(conditions, " &&\n"), pre="return ",
                          reindent=True))

Comment 30

[Reuben Morais [:reuben]]

It would be nice to have this information exposed in a way that allows for queries of the type "does this window have access to this interface?". That would allow us to have WebIDL as the single source of truth for that information, for example to be used by a navigator.hasFeature() API.

Comment 31

[Reuben Morais [:reuben]]

Hmm, it looks like that's already possible by querying nsScriptNameSpaceManager and calling mConstructorEnabled on the result with the global you want? Nice.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> it looks like that's already possible by querying nsScriptNameSpaceManager and calling
> mConstructorEnabled on the result with the global you want?

Yep.  That's how the window resolve hook goes about answering the question.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> I wish we could replace these with Func instead :-/.

We can, sure.  Just haven't gotten around to it.  I considered doing it in this bug, but there's a ton of uses for webaudio stuff and whatnot.  Then again, maybe they call call the same C++ method; I seem to recall ehsan setting it up that way...

> Or you could do something like:

Yeah, fair.  I did this to make it actually work:

        body = CGWrapper(CGList((CGGeneric(cond) for cond in conditions),
                                " &&\n"),
                         pre="return ", post=";", reindent=True)

Comment 34

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dcdc32aa5ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f15d12ba1c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8ad8975b67
https://hg.mozilla.org/integration/mozilla-inbound/rev/614d88a08207
https://hg.mozilla.org/integration/mozilla-inbound/rev/4153e70b0960

Comment 35

[Boris Zbarsky [:bzbarsky]]

Documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#AvailableIn

Comment 36

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #33)
> > I wish we could replace these with Func instead :-/.
> 
> We can, sure.  Just haven't gotten around to it.  I considered doing it in
> this bug, but there's a ton of uses for webaudio stuff and whatnot.  Then
> again, maybe they call call the same C++ method; I seem to recall ehsan
> setting it up that way...

Yes, but I'm removing all of that crap in bug 968479.  :-)

Comment 37

[Boris Zbarsky [:bzbarsky]]

I filed bug 968643 on nixing PrefControlled.

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5dcdc32aa5ed
https://hg.mozilla.org/mozilla-central/rev/5f15d12ba1c4
https://hg.mozilla.org/mozilla-central/rev/1c8ad8975b67
https://hg.mozilla.org/mozilla-central/rev/614d88a08207
https://hg.mozilla.org/mozilla-central/rev/4153e70b0960

Comment 39

[Reuben Morais [:reuben]]

So I'm trying to use [AvailableIn] on XMLHttpRequest.mozSystem, which is exposed to workers. The problem I'm running into is that IsInPrivilegedApp is always called before WorkerPrivate::SetPrincipal, so it's always false when we need it.

Comment 40

[(no longer active)]

(In reply to comment #39)
> So I'm trying to use [AvailableIn] on XMLHttpRequest.mozSystem, which is
> exposed to workers. The problem I'm running into is that IsInPrivilegedApp is
> always called before WorkerPrivate::SetPrincipal, so it's always false when we
> need it.

Hmm, I thought [AvailableIn] was tested to work in workers... :/