Make allowfullscreen and allowusermedia match implementations

[annevk]

Fullscreen sandboxing was never implemented and it appears there are no
immediate plans for it by either Google or Mozilla. So let’s remove it.
Fixes #1240.

Furthermore, the way allowfullscreen works in implementations is by
dynamically checking the attribute. That requires moving to an “allowed
to use” algorithm rather than using a fullscreen enabled flag. (Aside,
the fullscreen enabled flag logic got broken in
688df43 but that no longer matters.)
Fixes #1481.

This change also aligns the recently introduced allowusermedia with
that model since it seems unlikely implementers want to do something
different there.

[annevk]

Folks that should probably look at this include: @stefhak, @adam-be, @domenic, @upsuper, and maybe @foolip.

[upsuper]

We've implemented fullscreen sandboxing, but fine to remove it given no other browser does that.

[upsuper]

Why isn't allowusermedia a sandbox flag directly instead of adding something aligned with allowfullscreen attribute?

[annevk]

Presumably they want to allow it outside sandboxed <iframe> too?

[upsuper]

Isn't usermedia allowed by default if not sandboxed?

[annevk]

Presumably not in an <iframe>. Otherwise I don't understand why they added that attribute in the first place.

[upsuper]

I don't see anything in either our codebase or Chromium's codebase includes allowusermedia. I suppose no browser has been shipping this. Shouldn't we reconsider? If this is currently allowed by default, but then turned to being disallowed by default, wouldn't it be an web-compatibility issue?

[foolip]

There's a problem with walking the parent iframes when you have out-of-process iframes, you can't actually check that synchronously. There was some discussion about this in https://docs.google.com/document/d/1PkpVsDiGlpsGv4vbViHoa2zmzrjCWSy2ASwGl3KR0FE/edit?usp=sharing

[annevk]

@upsuper, this isn't really the place to have that discussion. The folks working on Media Capture and Streams made that call and I assume they have some reason as to why it's okay.

@foolip, numerous APIs have that problem today. I'm hoping those working on that have some idea as to how they are going to update the standards. For now though, this is how browsers implement this.

[upsuper]

@foolip If Chrome is fine to live with that compability issue, we can do so as well. And if you can ship that change to release in this year (not necessarily with unprefixed API), I would make the current state ride the train to the release. I don't think it is something worth adding another flag...

[foolip]

Let's not assume that everyone has considered all of the implications for compat. Does anyone know where that discussion was had for Media Capture and Streams? Without telemetry, it's hard to have any confidence that changing the default from allow to disallow will work out.

[foolip]

@foolip, numerous APIs have that problem today. I'm hoping those working on that have some idea as to how they are going to update the standards. For now though, this is how browsers implement this.

It looks like the way it was implemented in Chromium is to replicate the attribute values on a proxy object in the other process. I guess that's not possible to distinguish from what you've proposed here.

[upsuper]

@annevk OK, it seems Chrome disallows gUM in <iframe> by default, but Firefox allows... And it seems currently there is no way for a page in <iframe> to get user media at all in Chrome because allowusermedia is not implemented yet.

[annevk]

Let's not assume that everyone has considered all of the implications for compat.

This bug issue is not about changing defaults. It is only about removing the sandbox association and making the check dynamic, for both allow* attributes, to match implementations for allowfullscreen and match the intent for allowusermedia. It's a little frustrating to discuss all kinds of unrelated matters in a PR that is solely about improving compatibility with implementations.

[upsuper]

It looks like the way it was implemented in Chromium is to replicate the attribute values on a proxy object in the other process. I guess that's not possible to distinguish from what you've proposed here.

Oh, it seems Gecko has something similiar, and I guess this would work.

[upsuper]

OK, so this change looks good to me.

[domenic]

Can someone explain to me what the difference is between a dynamic check, as in this PR, and the sandboxing-based version---if we fixed all the bugs with copying the sandboxing flags around? I always thought the reason allowfullscreen/allowusermedia were stored in the same place as the sandbox flag was purely for spec organization purposes, and I still kind of prefer the idea of grouping them all together.

If there is a difference, I think explaining it would be an important part of the commit message here.

[domenic]

How would specs call this? Something like

If document is allowed to use the feature indicated by the attribute name "allowusermedia"

? Seems kind of verbose... I don't have any specific better suggestions though.

[annevk]

Yes.

[stefhak]

I had a quick look, and this PR seems fine to me (but I hope someone will check more thoroughly).
In the media capture tf we did discuss iFrames' access to camera/micprophone quite a bit. It started at TPAC time when we decided that when an iFrame's content asks for access to camera/microphone the UI presented to the user should expose the origin of the iFrame and of the top level document to the user, and further discussion led to that only iFrames that were explicitly given the right should be able to get access, i.e. allowusermedia. I could dig out the records of the discussion if there is need.
Sandboxed iFrames where mentioned too, but I don't remember specifics.

[annevk]

@domenic the difference is when the attribute is observed.

[domenic]

Can you be more specific? For example, showing some sample code that would cause different results?

[annevk]

Sure, just set the attribute post-load. Works in browsers, doesn't work per spec.

[domenic]

Ah I see, thanks. I think that would be good to include in the eventual commit message.

[annevk]

Updated the commit message.

[foolip]

Seems like there's confusion about the current default for getUserMedia so I did some testing of my own.

Here's my gum.html test file:

<!doctype html>
<button>getUserMedia</button>
<script>
if (!navigator.getUserMedia) {
  navigator.getUserMedia = navigator.mozGetUserMedia || navigator.webkitGetUserMedia;
}
document.querySelector('button').onclick = function(event) {
  navigator.getUserMedia({audio:true}, function() {
    event.target.textContent = 'OK';
  }, function() {
    event.target.textContent = 'ERROR';
  });
  event.target.textContent = '...';
  event.target.disabled = true;
}
</script>

In Chrome, embedding that in an iframe from a same-origin or cross-origin page works, AFAICT the only requirement is that it's a secure origin.

In Firefox same-origin works as well, but I wasn't able to test cross-origin because my other test site was on an internal network with a magic certificate.

I wasn't able to test Edge because it's running in a virtual machine with no camera or microphone.

(I see no particular reason to suspect differences between same-origin or cross-origin iframes, but it was worth testing.)

It doesn't seem safe to assume that making allowusermedia work analogously to allowfullscreen is actually going to work out.

[annevk]

It doesn't seem safe to assume that making allowusermedia work analogously to allowfullscreen is actually going to work out.

Again, isn't that an orthogonal issue?

[foolip]

The refactoring into a shared algorithm only makes sense with a hoped-for end state where allowfullscreen and allowusermedia work the same way, right?

This change looks good and indeed necessary for Fullscreen, but it seems somewhat unlikely to me that the allowusermedia attribute will actually end up working like this. It seems more likely that it will be removed entirely, even.

I will comment on #1211 and w3c/mediacapture-main#353

[foolip]

Looks like the original discussion was in w3c/mediacapture-main#268

[annevk]

Even if allowusermedia goes away, we might still want to keep the algorithm since we can use it for future fullscreen-like features that do consider this from the start.

[foolip]

Emphasis of no looks atypical.

[annevk]

It's somewhat common for "not" and I tend to use it myself quite a bit in other standards. I think it helps as typically there is no negation.

[foolip]

Hmm, I think this also opens up a hole for something like this in any context:

var doc = document.implementation.createHTMLDocument();
doc.documentElement.requestFullscreen();

Because doc doesn't have a browsing context container, I think this algorithm will return true here, and then what happens? That didn't work previously because the fullscreen enabled flag would be false in that case.

As implemented in Blink, there's an extra if (!document.frame()) return false check as the first step, but I'm not sure how to express that in the spec. Maybe this algorithm should only ever return true once the top-level browsing context is reached?

[annevk]

You're right, I should check that document has a browsing context, before checking properties of that browsing context.

[annevk]

Done.

[upsuper]

Even if allowusermedia goes away, we might still want to keep the algorithm since we can use it for future fullscreen-like features that do consider this from the start.

I don't think so.

[foolip]

Maybe it will come to future use, maybe not, but I'm OK with a bit more generality than strictly needed. Changes LGTM % nits, and I'll have to keep an eye on w3c/mediacapture-main#268 about the future of allowusermedia.

[upsuper]

I've posted why I don't think new things should go the fullscreen way in w3c/mediacapture-main#268 (comment).

[annevk]

@upsuper if you think we should block this PR on whether or not allowusermedia stays around I guess that's fine, but it does not seem great as allowfullscreen will remain incorrect for longer. (I don't think we can really fix allowfullscreen while keeping allowusermedia the way it is.)

@stefhak any idea when the community will figure out allowusermedia? Should we remove it from HTML for now?

[upsuper]

@annevk I don't think that needs to block this change, but that's up to you.

[domenic]

FWIW I think we should fix both at once and keep allowusermedia until the media capture folks come to consensus on a different model.

Today is still a day off for me so I'm happy to let others be the reviewers and sign-off.

[domenic]

These <em>no</em>s look pretty weird to me. Ctrl+Fing for "no browsing context" does not turn up any other instances.

[annevk]

Addressed feedback.