Bug 178754 – [MediaStream] Clear cached gUM prompt state

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Eric Carlson]

Clear a page's cached gUM prompt state, so the user will be prompted again on the next call to getUserMedia, after:

  - a stream has been active for 24 hours
  - no stream has been active for a platform specific interval (defaults to 1 minute on iOS, 10 minutes on other platforms, settable with a WK2 setting)

[Eric Carlson]

<rdar://problem/32742356>

[Eric Carlson]

Created attachment 324739 [details]
Proposed patch.

[youenn fablet]

Comment on attachment 324739 [details]
Proposed patch.

Patch lgtm, not applying so...

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

> Source/WebKit/Shared/WebPreferencesDefinitions.h:315
> +    macro(MediaCaptureInactiveStreamRepromptInterval, mediaCaptureInactiveStreamRepromptInterval, Double, double, DEFAULT_MEDIA_CAPTURE_INTERACTIVE_STREAM_REPROMPT_INTERVAL, "", "") \

Are we usually using minutes and not seconds for intervals/timeouts values?

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:414
> +        interval = Seconds::fromHours(24);

Shouldn't we try to put the default long timeout value in a better place, maybe in a macro in WebPreferenceDefinitions?

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:422
> +    m_watchdogTimer.startOneShot(m_currentWatchdogInterval);

Are there cases where we should stop the timer like in destructor or is it handled on its own?

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:434
> +    ASSERT(!(m_captureState & activeCaptureMask));

Cannot this assertion happens if there is a live webrtc session for more than 24 hours?

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:437
> +    m_pregrantedRequests.clear();

We should also clear m_deniedRequests.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:438
> +

line not needed

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:444
> +// Defaults to 8 minutes.

Is it still true?

[Eric Carlson]

Comment on attachment 324739 [details]
Proposed patch.

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

>> Source/WebKit/Shared/WebPreferencesDefinitions.h:315
>> +    macro(MediaCaptureInactiveStreamRepromptInterval, mediaCaptureInactiveStreamRepromptInterval, Double, double, DEFAULT_MEDIA_CAPTURE_INTERACTIVE_STREAM_REPROMPT_INTERVAL, "", "") \
> 
> Are we usually using minutes and not seconds for intervals/timeouts values?

I don't think there is a preference one way or the other, and I think expressing 5 minutes as "5*60" is helpful, but I changed the preference name to "inactiveMediaCaptureSteamRepromptIntervalInMinutes" to make the units obvious.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:414
>> +        interval = Seconds::fromHours(24);
> 
> Shouldn't we try to put the default long timeout value in a better place, maybe in a macro in WebPreferenceDefinitions?

I added a preference for it.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:422
>> +    m_watchdogTimer.startOneShot(m_currentWatchdogInterval);
> 
> Are there cases where we should stop the timer like in destructor or is it handled on its own?

A timer's destructor stops itself.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:434
>> +    ASSERT(!(m_captureState & activeCaptureMask));
> 
> Cannot this assertion happens if there is a live webrtc session for more than 24 hours?

Indeed, removed!

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:437
>> +    m_pregrantedRequests.clear();
> 
> We should also clear m_deniedRequests.

I disagree. If a user disallows capture, we should remember that setting until the page is reloaded.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:438
>> +
> 
> line not needed

Fixed.

>> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:444
>> +// Defaults to 8 minutes.
> 
> Is it still true?

Nope, fixed.

[Eric Carlson]

Created attachment 324860 [details]
Rebased and updated patch.

[youenn fablet]

Comment on attachment 324860 [details]
Rebased and updated patch.

LGTM.

I am still not sure whether we should not remove the denied requests decisions after some timeouts.
The timer will only fire if user granted either audio or video.
So there will be a prompt for one or the other.
Then, why not allowing the application to request both?

[Eric Carlson]

(In reply to youenn fablet from comment #6)
> Comment on attachment 324860 [details]
> Rebased and updated patch.
> 
> LGTM.
> 
> I am still not sure whether we should not remove the denied requests
> decisions after some timeouts.
> The timer will only fire if user granted either audio or video.
> So there will be a prompt for one or the other.
> Then, why not allowing the application to request both?

Lets land this and talk about the issue.

[WebKit Commit Bot]

Comment on attachment 324860 [details]
Rebased and updated patch.

Clearing flags on attachment: 324860

Committed r223988: <https://trac.webkit.org/changeset/223988>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

The API test added with this change is failing:
FAIL WebKit2.GetUserMediaReprompt

/Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKit/GetUserMediaReprompt.mm:80
Value of: [result boolValue]
  Actual: false
Expected: true


/Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKit/GetUserMediaReprompt.mm:89
Value of: [result boolValue]
  Actual: false
Expected: true

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/5281

[Eric Carlson]

Created attachment 325029 [details]
Test fix.

[Ryan Haddad]

Comment on attachment 325029 [details]
Test fix.

Clearing flags on attachment: 325029

Committed r224044: <https://trac.webkit.org/changeset/224044>

[Ryan Haddad]

All reviewed patches have been landed.  Closing bug.