On page close, WebPage::m_userMediaPermissionRequestManager is nullified too early
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Dec 2018 16:59:15 +0000 (16:59 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Dec 2018 16:59:15 +0000 (16:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192657

Reviewed by Eric Carlson.

Source/WebKit:

Instead of nullifying the manager, make it a UniqueRef and clear it on closing the page.
This ensures we revoke the sandbox extensions as early as possible and keep the manager lifetime simple.

* WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:
(WebKit::UserMediaPermissionRequestManager::~UserMediaPermissionRequestManager):
(WebKit::UserMediaPermissionRequestManager::clear):
* WebProcess/MediaStream/UserMediaPermissionRequestManager.h:
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::close):
* WebProcess/WebPage/WebPage.h:
(WebKit::WebPage::userMediaPermissionRequestManager):

Tools:

Add a test that loads a page registering ondevicechange,
load another page in the same process, closes the first page.
Ensure that the process does not crash in that case.

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WebKit/UserMedia.cpp:
(TestWebKitAPI::TEST):
(TestWebKitAPI::didCrashCallback):
* TestWebKitAPI/Tests/WebKit/ondevicechange.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239167 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp
Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Source/WebKit/WebProcess/WebPage/WebPage.h
Tools/ChangeLog
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WebKit/UserMedia.cpp
Tools/TestWebKitAPI/Tests/WebKit/ondevicechange.html [new file with mode: 0644]

index a719688..366a1d9 100644 (file)
@@ -1,3 +1,22 @@
+2018-12-13  Youenn Fablet  <youenn@apple.com>
+
+        On page close, WebPage::m_userMediaPermissionRequestManager is nullified too early
+        https://bugs.webkit.org/show_bug.cgi?id=192657
+
+        Reviewed by Eric Carlson.
+
+        Instead of nullifying the manager, make it a UniqueRef and clear it on closing the page.
+        This ensures we revoke the sandbox extensions as early as possible and keep the manager lifetime simple.
+
+        * WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:
+        (WebKit::UserMediaPermissionRequestManager::~UserMediaPermissionRequestManager):
+        (WebKit::UserMediaPermissionRequestManager::clear):
+        * WebProcess/MediaStream/UserMediaPermissionRequestManager.h:
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::close):
+        * WebProcess/WebPage/WebPage.h:
+        (WebKit::WebPage::userMediaPermissionRequestManager):
+
 2018-12-13  Chris Fleizach  <cfleizach@apple.com>
 
         [meta][WebKit] Remove using namespace WebCore and WebKit in the global scope for unified source builds
index acb99fc..ad21dd2 100644 (file)
@@ -53,6 +53,11 @@ UserMediaPermissionRequestManager::UserMediaPermissionRequestManager(WebPage& pa
 
 UserMediaPermissionRequestManager::~UserMediaPermissionRequestManager()
 {
+    clear();
+}
+
+void UserMediaPermissionRequestManager::clear()
+{
     for (auto& sandboxExtension : m_userMediaDeviceSandboxExtensions)
         sandboxExtension.value->revoke();
 }
index 6004290..851b7ee 100644 (file)
@@ -57,6 +57,7 @@ public:
     void removeDeviceChangeObserver(WebCore::UserMediaClient::DeviceChangeObserverToken);
 
     void captureDevicesChanged();
+    void clear();
 
 private:
     void sendUserMediaRequest(WebCore::UserMediaRequest&);
index 99a069c..cb719d4 100644 (file)
@@ -379,7 +379,7 @@ WebPage::WebPage(uint64_t pageID, WebPageCreationParameters&& parameters)
     , m_geolocationPermissionRequestManager(makeUniqueRef<GeolocationPermissionRequestManager>(*this))
 #endif
 #if ENABLE(MEDIA_STREAM)
-    , m_userMediaPermissionRequestManager { std::make_unique<UserMediaPermissionRequestManager>(*this) }
+    , m_userMediaPermissionRequestManager { makeUniqueRef<UserMediaPermissionRequestManager>(*this) }
 #endif
     , m_pageScrolledHysteresis([this](PAL::HysteresisState state) { if (state == PAL::HysteresisState::Stopped) pageStoppedScrolling(); }, pageScrollHysteresisDuration)
     , m_canRunBeforeUnloadConfirmPanel(parameters.canRunBeforeUnloadConfirmPanel)
@@ -1221,7 +1221,7 @@ void WebPage::close()
     m_page->inspectorController().disconnectAllFrontends();
 
 #if ENABLE(MEDIA_STREAM)
-    m_userMediaPermissionRequestManager = nullptr;
+    m_userMediaPermissionRequestManager->clear();
 #endif
 
 #if ENABLE(FULLSCREEN_API)
index d4a9b7d..e0af2f6 100644 (file)
@@ -576,7 +576,7 @@ public:
 #endif
 
 #if ENABLE(MEDIA_STREAM)
-    UserMediaPermissionRequestManager& userMediaPermissionRequestManager() { return *m_userMediaPermissionRequestManager; }
+    UserMediaPermissionRequestManager& userMediaPermissionRequestManager() { return m_userMediaPermissionRequestManager; }
     void prepareToSendUserMediaPermissionRequest();
     void captureDevicesChanged();
 #endif
@@ -1626,7 +1626,7 @@ private:
 #endif
 
 #if ENABLE(MEDIA_STREAM)
-    std::unique_ptr<UserMediaPermissionRequestManager> m_userMediaPermissionRequestManager;
+    UniqueRef<UserMediaPermissionRequestManager> m_userMediaPermissionRequestManager;
 #endif
 
     std::unique_ptr<WebCore::PrintContext> m_printContext;
index 6ef2949..0958aed 100644 (file)
@@ -1,3 +1,20 @@
+2018-12-13  Youenn Fablet  <youenn@apple.com>
+
+        On page close, WebPage::m_userMediaPermissionRequestManager is nullified too early
+        https://bugs.webkit.org/show_bug.cgi?id=192657
+
+        Reviewed by Eric Carlson.
+
+        Add a test that loads a page registering ondevicechange,
+        load another page in the same process, closes the first page.
+        Ensure that the process does not crash in that case.
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/WebKit/UserMedia.cpp:
+        (TestWebKitAPI::TEST):
+        (TestWebKitAPI::didCrashCallback):
+        * TestWebKitAPI/Tests/WebKit/ondevicechange.html: Added.
+
 2018-12-13  Carlos Eduardo Ramalho  <cadubentzen@gmail.com>
 
         [GStreamer][JHBuild] update-webkit{gtk,wpe}-libs fails with libfdk-2.0.0
index a89e05b..7e8ed3b 100644 (file)
@@ -26,6 +26,7 @@
                07492B3B1DF8B14C00633DE1 /* EnumerateMediaDevices.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 07492B3A1DF8AE2D00633DE1 /* EnumerateMediaDevices.cpp */; };
                07492B3C1DF8B86600633DE1 /* enumerateMediaDevices.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 07492B391DF8ADA400633DE1 /* enumerateMediaDevices.html */; };
                074994421EA5034B000DA44E /* getUserMedia.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 4A410F4D19AF7BEF002EBAB5 /* getUserMedia.html */; };
+               074994421EA5034B000DA44F /* ondevicechange.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 4A410F4D19AF7BEF002EBAB6 /* ondevicechange.html */; };
                076E507F1F4513D6006E9F5A /* Logging.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 076E507E1F45031E006E9F5A /* Logging.cpp */; };
                0799C3491EBA2D7B003B7532 /* UserMediaDisabled.mm in Sources */ = {isa = PBXBuildFile; fileRef = 07EDEFAC1EB9400C00D43292 /* UserMediaDisabled.mm */; };
                0799C34B1EBA3301003B7532 /* disableGetUserMedia.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 0799C34A1EBA32F4003B7532 /* disableGetUserMedia.html */; };
                                466C3843210637DE006A88DE /* notify-resourceLoadObserver.html in Copy Resources */,
                                CDB5DFFF213610FA00D3E189 /* now-playing.html in Copy Resources */,
                                93E2D2761ED7D53200FA76F6 /* offscreen-iframe-of-media-document.html in Copy Resources */,
+                               074994421EA5034B000DA44F /* ondevicechange.html in Copy Resources */,
                                CEA6CF2819CCF69D0064F5A7 /* open-and-close-window.html in Copy Resources */,
                                7CCB99231D3B4A46003922F6 /* open-multiple-external-url.html in Copy Resources */,
                                290A9BB91735F63800D71BBC /* OpenNewWindow.html in Copy Resources */,
                46E816F71E79E29100375ADC /* RestoreStateAfterTermination.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = RestoreStateAfterTermination.mm; sourceTree = "<group>"; };
                4A410F4B19AF7BD6002EBAB5 /* UserMedia.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = UserMedia.cpp; sourceTree = "<group>"; };
                4A410F4D19AF7BEF002EBAB5 /* getUserMedia.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = getUserMedia.html; sourceTree = "<group>"; };
+               4A410F4D19AF7BEF002EBAB6 /* ondevicechange.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = ondevicechange.html; sourceTree = "<group>"; };
                4BB4160116815B2600824238 /* JSWrapperForNodeInWebFrame.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = JSWrapperForNodeInWebFrame.mm; sourceTree = "<group>"; };
                4BB4160316815F9100824238 /* ElementAtPointInWebFrame.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ElementAtPointInWebFrame.mm; sourceTree = "<group>"; };
                4BFDFFA61314776C0061F24B /* HitTestResultNodeHandle_Bundle.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = HitTestResultNodeHandle_Bundle.cpp; sourceTree = "<group>"; };
                                33E79E05137B5FCE00E32D99 /* mouse-move-listener.html */,
                                5797FE321EB15A8900B2F4A0 /* navigation-client-default-crypto.html */,
                                C99B675E1E39735C00FC6C80 /* no-autoplay-with-controls.html */,
+                               4A410F4D19AF7BEF002EBAB6 /* ondevicechange.html */,
                                CEA6CF2719CCF69D0064F5A7 /* open-and-close-window.html */,
                                83148B08202AC76800BADE99 /* override-builtins-test.html */,
                                0EBBCC651FFF9DCE00FA42AB /* pop-up-check.html */,
                                A155022A1E05020B00A24C57 /* DuplicateCompletionHandlerCalls.mm in Sources */,
                                7CCE7EBE1A411A7E00447C4C /* DynamicDeviceScaleFactor.mm in Sources */,
                                5C0BF8921DD599B600B00328 /* EarlyKVOCrash.mm in Sources */,
-                               52D5D6C021B9F1B30046ABA6 /* RenderingProgress.mm in Sources */,
                                F44D064A1F3962F2001A0E29 /* EditingTestHarness.mm in Sources */,
                                7CCE7EE01A411A9A00447C4C /* EditorCommands.mm in Sources */,
                                F44D06471F39627A001A0E29 /* EditorStateTests.mm in Sources */,
                                7CCE7EC91A411A7E00447C4C /* RenderedImageFromDOMNode.mm in Sources */,
                                7CCE7ECA1A411A7E00447C4C /* RenderedImageFromDOMRange.mm in Sources */,
                                A12DDBFB1E836F0700CF6CAE /* RenderedImageWithOptions.mm in Sources */,
+                               52D5D6C021B9F1B30046ABA6 /* RenderingProgress.mm in Sources */,
                                F464AF9220BB66EA007F9B18 /* RenderingProgressTests.mm in Sources */,
                                7C83E0C41D0A654200FEBCF3 /* RequiresUserActionForPlayback.mm in Sources */,
                                7CCE7F0E1A411AE600447C4C /* ResizeReversePaginatedWebView.cpp in Sources */,
                                0E404A8C2166DE0A008271BA /* InjectedBundleNodeHandleIsSelectElement.mm in Sources */,
                                79C5D431209D768300F1E7CA /* InjectedBundleNodeHandleIsTextField.mm in Sources */,
                                F44C7A0020F9EEBF0014478C /* ParserYieldTokenPlugIn.mm in Sources */,
-                               5245178721B9F57B0082CB34 /* RenderingProgressPlugIn.mm in Sources */,
                                A13EBBAB1B87434600097110 /* PlatformUtilitiesCocoa.mm in Sources */,
                                1A4F81CF1BDFFD53004E672E /* RemoteObjectRegistryPlugIn.mm in Sources */,
                                A12DDC021E837C2400CF6CAE /* RenderedImageWithOptionsPlugIn.mm in Sources */,
+                               5245178721B9F57B0082CB34 /* RenderingProgressPlugIn.mm in Sources */,
                                7C882E091C80C630006BF731 /* UserContentWorldPlugIn.mm in Sources */,
                                7C83E03D1D0A60D600FEBCF3 /* UtilitiesCocoa.mm in Sources */,
                                A13EBBAA1B87428D00097110 /* WebProcessPlugIn.mm in Sources */,
index 363b753..2f0cbe2 100644 (file)
@@ -35,6 +35,7 @@
 
 namespace TestWebKitAPI {
 
+static bool didCrash;
 static bool wasPrompted;
 
 static void decidePolicyForUserMediaPermissionRequestCallBack(WKPageRef, WKFrameRef, WKSecurityOriginRef, WKSecurityOriginRef, WKUserMediaPermissionRequestRef permissionRequest, const void* /* clientInfo */)
@@ -95,6 +96,67 @@ TEST(WebKit, UserMediaBasic)
     Util::run(&wasPrompted);
 }
 
+static void didCrashCallback(WKPageRef, const void*)
+{
+    didCrash = true;
+    // Set wasPrompted to true to speed things up, we know the test failed.
+    wasPrompted = true;
+}
+
+TEST(WebKit, OnDeviceChangeCrash)
+{
+    auto context = adoptWK(WKContextCreate());
+
+    WKRetainPtr<WKPageGroupRef> pageGroup(AdoptWK, WKPageGroupCreateWithIdentifier(Util::toWK("GetUserMedia").get()));
+    WKPreferencesRef preferences = WKPageGroupGetPreferences(pageGroup.get());
+    WKPreferencesSetMediaDevicesEnabled(preferences, true);
+    WKPreferencesSetFileAccessFromFileURLsAllowed(preferences, true);
+    WKPreferencesSetMediaCaptureRequiresSecureConnection(preferences, false);
+    WKPreferencesSetMockCaptureDevicesEnabled(preferences, true);
+
+    WKPageUIClientV6 uiClient;
+    memset(&uiClient, 0, sizeof(uiClient));
+    uiClient.base.version = 6;
+    uiClient.decidePolicyForUserMediaPermissionRequest = decidePolicyForUserMediaPermissionRequestCallBack;
+    uiClient.checkUserMediaPermissionForOrigin = checkUserMediaPermissionCallback;
+
+    PlatformWebView webView(context.get(), pageGroup.get());
+    WKPageSetPageUIClient(webView.page(), &uiClient.base);
+
+    // Load a page registering ondevicechange handler.
+    auto url = adoptWK(Util::createURLForResource("ondevicechange", "html"));
+    ASSERT(url.get());
+
+    WKPageLoadURL(webView.page(), url.get());
+
+    // Load a second page in same process.
+    PlatformWebView webView2(context.get(), pageGroup.get());
+    WKPageSetPageUIClient(webView2.page(), &uiClient.base);
+    WKPageLoaderClientV0 loaderClient;
+    memset(&loaderClient, 0, sizeof(loaderClient));
+    loaderClient.base.version = 0;
+    loaderClient.processDidCrash = didCrashCallback;
+    WKPageSetPageLoaderClient(webView2.page(), &loaderClient.base);
+
+    wasPrompted = false;
+    url = adoptWK(Util::createURLForResource("getUserMedia", "html"));
+    WKPageLoadURL(webView2.page(), url.get());
+    Util::run(&wasPrompted);
+    EXPECT_EQ(WKPageGetProcessIdentifier(webView.page()), WKPageGetProcessIdentifier(webView2.page()));
+
+    didCrash = false;
+
+    // Close first page.
+    WKPageClose(webView.page());
+
+    wasPrompted = false;
+    url = adoptWK(Util::createURLForResource("getUserMedia", "html"));
+    WKPageLoadURL(webView2.page(), url.get());
+    Util::run(&wasPrompted);
+    // Verify pages process did not crash.
+    EXPECT_TRUE(!didCrash);
+}
+
 } // namespace TestWebKitAPI
 
 #endif // ENABLE(MEDIA_STREAM)
diff --git a/Tools/TestWebKitAPI/Tests/WebKit/ondevicechange.html b/Tools/TestWebKitAPI/Tests/WebKit/ondevicechange.html
new file mode 100644 (file)
index 0000000..b7a8081
--- /dev/null
@@ -0,0 +1,4 @@
+<!DOCTYPE html>
+<script>
+navigator.mediaDevices.ondevicechange = () => { };
+</script>