Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a SameRealmBrandCheck extended attribute #718

Closed
wants to merge 1 commit into from

Conversation

Ms2ger
Copy link
Member

@Ms2ger Ms2ger commented Apr 26, 2019

@Ms2ger
Copy link
Member Author

Ms2ger commented Apr 26, 2019

@domenic will be better able to argue its usefulness.

@bzbarsky
Copy link
Collaborator

Apart from the usefulness, I don't understand the semantics this is trying to define. That is, if I had to implement this, I have no idea what I would actually need to do.

index.bs Show resolved Hide resolved
@littledan
Copy link
Collaborator

One possibility that @domenic and I discussed for this is that it could be more of an implicit mode switch within a module ("interfaces declared inside modules are always this way, as a new default") than an explicit extended attribute.

@domenic
Copy link
Member

domenic commented Apr 26, 2019

As for usefulness, I think the spec text helps capture this. We'd like to reduce the complexity and un-polyfillable magic in some new specs we're working on.

@bzbarsky, the semantics of checking the realm seem pretty clear. What could we do to make them clearer?

I agree it's a bit tricky to use "implements" as the point at which to change this, but it is a very nice place because it covers everything. (Operations, attributes, incoming parameters, ...) Maybe we could do a quick audit of usage sites of "implements", or just have a backup where if there is no current Realm then we don't do that check.

Otherwise, I have the same issues as in #719 (comment), that we may want to signpost this as experimental or just make it part of being in a module, since that's a nice clean break point.

@bzbarsky
Copy link
Collaborator

That is, if I had to implement this, I have no idea what I would actually need to do.

Ah, I see what the actual requirements are; I had missed them when I was looking over the commit somehow.

It is in fact not clear to me whether there's always a current realm when doing brand checks. That would have to be pretty carefully audited.

I'd like to understand how this reduces complexity in specs, because this definitely increases complexity in implementations as far as I can tell. Can you point me to a specific example? Is this just about making sure the specs in question can be polyfilled with 100% fidelity? Quite honestly, my preferred answer there would be to expose the existing brand check to script.

@domenic
Copy link
Member

domenic commented Apr 26, 2019

I'd like to understand how this reduces complexity in specs, because this definitely increases complexity in implementations as far as I can tell.

Hmm, talking to @tschneidereit recently he implied it would decrease the complexity, especially given Gecko's multi-globals implementation. For example, it allows us to entirely avoid worrying about current realm vs. relevant realm! It also enables self-hosting of built-ins in JavaScript more easily.

Quite honestly, my preferred answer there would be to expose the existing brand check to script.

I'm not sure how that would work for the polyfilled class itself. For example, given some interface Foo with method bar, how would you polyfill that window1.Foo.prototype.bar.call(fooFromWindow2) works, but window1.Foo.prototype.bar.call(notAFoo) does not work?

@littledan had some ideas based on, e.g., using the script URL as a source of identity across realms in https://github.com/littledan/serializable-objects, in particular https://github.com/littledan/serializable-objects#class-identifier-tuple. But those all seemed pretty complex to me.

@littledan
Copy link
Collaborator

it allows us to entirely avoid worrying about current realm vs. relevant realm

I have to say, I'm really excited about this part. It would be very difficult to make polyfills in JavaScript that return relevant realm objects (since JS very widely closes over the realm of the method, i.e., the current realm), but @bzbarsky has made strong arguments in issues in this repository that relevant realm is sometimes the right one. With same-realm brand checks, these realms coincide!

@littledan had some ideas based on, e.g., using the script URL as a source of identity across realms

I'm not sure whether any of those ideas would really work at all; it would take a bit more thought. And from JS, we'd need some new language mechanism to tap into it (a built-in decorator??). Another idea that @tschneidereit suggested was to just allow a user-supplied string or Symbol.for-like mechanism end up determining the brand, sort of like COM GUIDs (that is, make it entirely spoof-able from JS, if you know the incantation, but maybe built-ins would have their own separate registry which would reduce that risk).

@bzbarsky
Copy link
Collaborator

he implied it would decrease the complexity

It requires new codepahts in bindings and JITs, no? I don't see how that decreases complexity. It does slow down various codepaths involving bindings, by adding an extra branch on this state, which is not great. I've been trying to think about how to eliminate that and haven't come up with something workable, but I keep hoping I will.

As you point out, it might decrease complexity in some API implementations, because relevant realm and current realm are the same. I agree that this is a nice benefit.

On the other hand, it increases the mental overhead for consumers a bit, because now these APIs act like nothing else in the web's standard library (either WebIDL-defined or ES-defined)... And people probably shouldn't do the .call() thing, but in some cases they do...

how would you polyfill that

So the real question here is how to ensure that the branding window1 and window2 use is compatible, right? And specifically, what's being polyfilled is the entire interface, not just one method?

If we want something guaranteed non-spoofable, I haven't thought of a good option here yet.

@tschneidereit
Copy link

Hmm, talking to @tschneidereit recently he implied it would decrease the complexity, especially given Gecko's multi-globals implementation. For example, it allows us to entirely avoid worrying about current realm vs. relevant realm! It also enables self-hosting of built-ins in JavaScript more easily.

What we were discussing was the Streams spec, where our implementation would be vastly simpler if one couldn't create a reader in realm A to target a stream in realm B. We wouldn't need to do all the unwrapping and checking for dead wrappers in that case.

@Ms2ger
Copy link
Member Author

Ms2ger commented Apr 29, 2019

We definitely should review all the places that use "implements" if we decide that this is a feature we want; I haven't done that yet. There are a bunch of places that use "object implements an interface that ...", which we may want to distinguish somehow. We do need to be careful, though, not to expose any feature that will allow cross-realm brand checking—any single one would defeat the point of this feature.

I do think it would be somewhat strange to silently key this off "is defined in a module", but then again, that may end up being the de facto situation anyway. We may wish to add some guidance about when to use this feature.

@bzbarsky
Copy link
Collaborator

if one couldn't create a reader in realm A to target a stream in realm B

Ah, yes, that makes sense. Of course that doesn't require the SameRealmBrandCheck bit, though it would be a simple way to implement that constraint.

Is there a plan to apply something like SameRealmBrandCheck to streams?

@bzbarsky
Copy link
Collaborator

(And to be clear, the streams implementation issue is precisely that it allocates things in the current Realm and sticks them in a slot on an object, instead of allocating said things in the object's relevant Realm...)

@tschneidereit
Copy link

Is there a plan to apply something like SameRealmBrandCheck to streams?

My assumption was that it'd be too late to do this, but @domenic expressed some hope that it might still be doable, so who knows. If it is possible, I'd certainly be in favor :)

@annevk
Copy link
Member

annevk commented Apr 30, 2020

@Ms2ger @domenic still interested in pursuing this?

@domenic
Copy link
Member

domenic commented Apr 30, 2020

No, I'd be okay closing this.

@Ms2ger Ms2ger closed this Apr 30, 2020
@annevk annevk deleted the SameRealmBrandCheck branch April 30, 2020 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants