getUserMedia requests should be processed sequentially in UIProcess

[youenn fablet]

getUserMedia requests should be processed sequentially in UIProcess

[youenn fablet]

Created attachment 371077 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/51311420>

[EWS Watchlist]

Comment on attachment 371077 [details]
Patch

Attachment 371077 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12342446

New failing tests:
http/wpt/service-workers/service-worker-networkprocess-crash.html

[EWS Watchlist]

Created attachment 371082 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[youenn fablet]

Created attachment 371090 [details]
Patch

[youenn fablet]

Created attachment 371209 [details]
Patch

[Eric Carlson]

Comment on attachment 371209 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        getUserMedia requests should be processed sequentially in UIProcess
> +        https://bugs.webkit.org/show_bug.cgi?id=198430
> +        <rdar://problem/51311420>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * UIProcess/UserMediaPermissionRequestManagerProxy.cpp:

Oops - double Changelogs.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:378
> +        if (m_currentUserMediaRequest != request)
>              return;

How can this happen? It is probably worth logging an error if this should never happen.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:400
> +        if (!weakThis || m_currentUserMediaRequest != request)

Ditto.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:404
> +            if (!weakThis || m_currentUserMediaRequest != request)

Ditto.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:414
> +            if (!weakThis || m_currentUserMediaRequest != request)

Ditto.

[youenn fablet]

> > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:378
> > +        if (m_currentUserMediaRequest != request)
> >              return;
> 
> How can this happen? It is probably worth logging an error if this should
> never happen.

This can happen in this case:
- a request is being processed and is doing some async tasks, like prompting user
- the manager proxy invalidates all requests (for instance the app sets setCaptureEnabled to false, then back to true)
- the page triggers a new request which then becomes the new current request.

I'll make it clearer.

[youenn fablet]

Created attachment 371342 [details]
Patch for landing

[youenn fablet]

Created attachment 371347 [details]
Patch for landing

[youenn fablet]

Created attachment 371348 [details]
Fix windows

[WebKit Commit Bot]

Comment on attachment 371348 [details]
Fix windows

Clearing flags on attachment: 371348

Committed r246093: <https://trac.webkit.org/changeset/246093>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Hm:

[187/322] Building CXX object Source/W...sources/UnifiedSource-88d1702b-3.cpp.o
In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-88d1702b-3.cpp:6:
../../Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp: In member function ‘void WebKit::UserMediaPermissionRequestManagerProxy::requestUserMediaPermissionForFrame(uint64_t, uint64_t, WTF::Ref<WebCore::SecurityOrigin>&&, WTF::Ref<WebCore::SecurityOrigin>&&, WebCore::MediaStreamRequest&&)’:
../../Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:336:10: warning: variable ‘logSiteIdentifier’ set but not used [-Wunused-but-set-variable]
  336 |     auto logSiteIdentifier = LOGIDENTIFIER;
      |          ^~~~~~~~~~~~~~~~~

Problem is that ALWAYS_LOG is of course only defined on Mac. This is annoying because the warning is ugly to silence.

[Michael Catanzaro]

Youenn, would you be OK with:

diff --git a/Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp b/Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp
index b1cf20cb787..7568deeee67 100644
--- a/Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp
+++ b/Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp
@@ -334,6 +334,9 @@ void UserMediaPermissionRequestManagerProxy::requestUserMediaPermissionForFrame(
 {
 #if ENABLE(MEDIA_STREAM)
     auto logSiteIdentifier = LOGIDENTIFIER;
+#if RELEASE_LOG_DISABLED
+    UNUSED_VARIABLE(logSiteIdentifier);
+#endif
 
     if (!m_page.hasRunningProcess())
         return;


Or can you think of a better way to write this?

[youenn fablet]

Looking again at it, we probably no longer need logSiteIdentifier.
I'll remove it tomorrow.