Bug 173104 – getUserMedia should not be prompted again if user denied access

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[youenn fablet]

At least until reloading/navigating.

[youenn fablet]

Created attachment 312314 [details]
Patch

[Geoffrey Garen]

Comment on attachment 312314 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312314&action=review

r=me with one change

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:208
> +bool UserMediaPermissionRequestManagerProxy::isRequestDenied(uint64_t frameID, const WebCore::SecurityOrigin& userMediaDocumentOrigin, const WebCore::SecurityOrigin& topLevelDocumentOrigin, bool needsAudio, bool needsVideo)

I would call this "wasRequestDenied" because it's a record of something that happened in the past, and not necessarily a comment on what is or should be happening right now.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:216
> +        if (deniedRequest.frameID != frameID)
> +            continue;

This seems wrong to me. A page could re-prompt the user infinitely by inserting and removing iframes with getUserMedia calls. Once the user says no to a given domain, that should be the end of it.

[youenn fablet]

Created attachment 312322 [details]
Patch

[youenn fablet]

<rdar://problem/32381046>

[youenn fablet]

Thanks for the review.

> > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:216
> > +        if (deniedRequest.frameID != frameID)
> > +            continue;
> 
> This seems wrong to me. A page could re-prompt the user infinitely by
> inserting and removing iframes with getUserMedia calls. Once the user says
> no to a given domain, that should be the end of it.

This departs from the current model, which I think should be simplified.
We can add back the flexibility if we discover the need for it.

This would mean:
- Only care about the main frame for both predenied/pregranted requests
- Only care about the main frame security origin at UIProcess/Client level, frames with different origins are not allowed to do gum calls anyway.
- Reflect in client API that we are only searching for camera/mic access, not per device choice.
We should reflect all of that in our IPC and Client APIs.

I will update the patch with just the deny case now.

[youenn fablet]

Created attachment 312326 [details]
Patch for landing

[youenn fablet]

Created attachment 312333 [details]
Patch for landing

[Geoffrey Garen]

> This departs from the current model, which I think should be simplified.
>
> We can add back the flexibility if we discover the need for it.
> 
> This would mean:
> - Only care about the main frame for both predenied/pregranted requests
> - Only care about the main frame security origin at UIProcess/Client level,
> frames with different origins are not allowed to do gum calls anyway.

I'm not an expert on how WebRTC permissions usually work. I wonder what other browsers do.

Other web APIs like storage and geolocation grant or deny permission per domain, rather than per frame or per function call.

I can see some value in prompting more for WebRTC than for storage or geolocation, since we consider storage and geolocation to be optional APIs whose absence shouldn't break a site, whereas absence of WebRTC will probably break a WebRTC site.

My intuition is that one prompt per main document is probably the right policy to start with.

[WebKit Commit Bot]

Comment on attachment 312333 [details]
Patch for landing

Clearing flags on attachment: 312333

Committed r217945: <http://trac.webkit.org/changeset/217945>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

(In reply to WebKit Commit Bot from comment #9)
> Comment on attachment 312333 [details]
> Patch for landing
> 
> Clearing flags on attachment: 312333
> 
> Committed r217945: <http://trac.webkit.org/changeset/217945>

This broke the GTK+ build with GCC-4.9 : https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/2570/steps/compile-webkit/logs/stdio

I suspect its related to the same issue with GCC-4.9 that Yusuke fixed 2 days ago https://trac.webkit.org/changeset/217829/

[youenn fablet]

(In reply to Carlos Alberto Lopez Perez from comment #11)
> (In reply to WebKit Commit Bot from comment #9)
> > Comment on attachment 312333 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 312333
> > 
> > Committed r217945: <http://trac.webkit.org/changeset/217945>
> 
> This broke the GTK+ build with GCC-4.9 :
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20Debian%20Stable%20%28Build%29/builds/2570/steps/compile-
> webkit/logs/stdio
> 
> I suspect its related to the same issue with GCC-4.9 that Yusuke fixed 2
> days ago https://trac.webkit.org/changeset/217829/

Ah, sorry about that. I won't be able to fix it until tomorrow...

[youenn fablet]

Looking at Yusuke patch, you might indeed be right. If nobody beats me, I'll try to fix it tomorrow

[Carlos Alberto Lopez Perez]

(In reply to youenn fablet from comment #13)
> Looking at Yusuke patch, you might indeed be right. If nobody beats me, I'll
> try to fix it tomorrow

(In reply to youenn fablet from comment #12)
> (In reply to Carlos Alberto Lopez Perez from comment #11)
> > (In reply to WebKit Commit Bot from comment #9)
> > > Comment on attachment 312333 [details]
> > > Patch for landing
> > > 
> > > Clearing flags on attachment: 312333
> > > 
> > > Committed r217945: <http://trac.webkit.org/changeset/217945>
> > 
> > This broke the GTK+ build with GCC-4.9 :
> > https://build.webkit.org/builders/GTK%20Linux%2064-
> > bit%20Release%20Debian%20Stable%20%28Build%29/builds/2570/steps/compile-
> > webkit/logs/stdio
> > 
> > I suspect its related to the same issue with GCC-4.9 that Yusuke fixed 2
> > days ago https://trac.webkit.org/changeset/217829/
> 
> Ah, sorry about that. I won't be able to fix it until tomorrow...

No problem: The GTK+ EWS and the GTK+ release and debug bots run a more recent GCC version (6.3).


We have this bot with GCC 4.9 because its our minimum version supported of the compiler, so we can test that WebKit remains build-able with that.

So, no hurries at all.

[Claudio Saavedra]

I think in this particular case the default initializers are redundant so it's safe to remove them. The struct is only initialized in one place with all members explicitly. I've committed https://trac.webkit.org/changeset/217974/webkit, please feel free to change if you disagree and prefer to have the default initializers and a constructor as well.

[Michael Catanzaro]

All struct or class members should always be initialized by default or by the constructor, we should not be playing games with possible use of uninitialized memory. The code instantiating the struct should not have to remember to initialize everything. We know this does not work in practice.