Should navigator.productSub/vendor/vendorSub be exposed to workers?

[foolip]

Updating Blink to the recent NavigatorID changes, it makes productSub, vendor and vendorSub exposed on Worker where it previously was not. I've tested with this script:

['appCodeName',
 'appName',
 'appVersion',
 'platform',
 'product',
 'productSub',
 'userAgent',
 'vendor',
 'vendorSub'].forEach(function(attr) {
     console.log(attr + ': ' + (attr in navigator ? 'exposed' : 'NOT exposed'));
 });

Blink (Opera) and Gecko (Firefox) both agree that productSub/vendor/vendorSub are not exposed on workers.

Should we limit these to Window in the spec as well, or just go ahead and expose them?

CC @bzbarsky

[bzbarsky]

I'm all in favor of not exposing this legacy cruft in workers....

[foolip]

I have tested Edge. All of the tested attributes are exposed on Window, as expected, but all except appCodeName are exposed on Worker :(

[annevk]

@travisleithead can you fix that?

[travisleithead]

Internal Edge bug filed (reference: VSO4790009). Thanks for the tip!

[foolip]

@travisleithead, any thoughts on what to do with productSub, vendor and vendorSub?

The one thing that bugs me is that it's going to look entirely arbitrary which attributes are exposed and not. It's pretty likely that more of this legacy cruft could be hidden from workers, but the effort to measure that hardly seems worth it.

[domenic]

@foolip it seems clear that we should not expose those three in workers? If Blink and Gecko agree and Edge is interested in aligning, then why not split them off?

[foolip]

I've now also tested in Safari and it exposes appName, appVersion, platform, and userAgent, but does not expose appCodeName, product, productSub, vendor and vendorSub.

Adding all of this up, it seems pretty likely that it's web-compatible not not expose appCodeName, product, productSub, vendor and vendorSub to workers, so how about trying that and changing only if it turns out that one of them is in fact needed?

[domenic]

SGTM

[domenic]

Someone should write web-platform-tests for this too :)

[foolip]

Oops, bad news. It's invalid per WebIDL to use [Exposed=Window] on a NavigatorID member, because WorkerNavigator has only [Exposed=Worker] and implements NavigatorID. The exposure set of a member must be a subset of the interface it's on.

This ought to be a problem for taintEnabled() as well.

Ideas?

[domenic]

It would presumably work to split things up into a few different mixin interfaces, right?

[annevk]

Merge Navigator and WorkerNavigator? Although I guess that might be incompatible as well because WorkerNavigator would no longer exist. Failing that, put the Window-only stuff on its own mixin.

[bzbarsky]

Why is that invalid?

There are three interfaces here: Navigator, WorkerNavigator, and NavigatorID. These are exposed on Window, Worker, and (Window, Worker) respectively. The relevant spec statements here are:

If [Exposed] appears on both an interface and one of its interface members, then the interface member's exposure set MUST be a subset of the interface's exposure set.

This is satisfied for members of NavigatorID that have [Exposed=Window], since the exposure set of NavigatorID includes Window.

An interface's exposure set MUST be a subset of the exposure set of all of the interface's consequential interfaces.

This is satisfied for both Navigator and WorkerNavigator.

So what is the problem exactly?

But yes, simply using two mixins would also work, and would be robust even in the face of changes to IDL that attempt to make the "mixin being mixed into interfaces with different exposure sets contains things that would only be exposed in one of them" pattern illegal. Then again, I believe the current spec is explicitly written to allow that pattern.

[Ms2ger]

Inlining the Window-only members in the definition of Navigator seems simplest, or partial if we want to hide them.

[foolip]

@bzbarsky, I didn't check what the WebIDL spec says before writing, I just got an error from Blink's IDL compiler and assumed it was right about this.

I don't have the error in front of me, but I think Blink's IDL compiler treats the [Exposed=Window] attribute as a member of WorkerNavigator, and then this rule from the spec doesn't hold:

If [Exposed] appears on both an interface and one of its interface members, then the interface member's exposure set must be a subset of the interface's exposure set.

How is this supposed to work with A implements B? It would be pretty strange if A is [Exposed=Window] but all of B's members are [Exposed=Worker], but maybe this is just nonsensical and not invalid?

[bzbarsky]

How is this supposed to work with A implements B?

Like I said in #216 (comment): The check is done against the interface the member is actually defined on, which is NavigatorID.

but maybe this is just nonsensical and not invalid?

That is the situation right now, yes.

I do think that the simplest thing to do in this situation is to put the non-shared stuff on Navigator directly or a partial interface Navigator as @Ms2ger suggests. The point of mixins is that you want to mix them into multiple things, so if we only want this property in one place we should just put it there.

[foolip]

Thanks, reopening so that I remember to fix this somehow.

[annevk]

Putting them directly on Navigator seems fine, but it also sounds like what we have now is fine, no?

[foolip]

Yeah, if this is valid per Web IDL I guess we could keep it the way it is. Closing again, but if anyone thinks it's too confusing like this or wants to lobby for Web IDL to make it invalid, just say the word.