Attachment 357209 Details for Bug 192657 – Patch

[patch] Patch

bug-192657-20181212223042.patch (text/plain), 14.83 KB, created by youenn fablet on 2018-12-12 22:30:38 PST

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2018-12-12 22:30:38 PST

Size: 14.83 KB

patch

obsolete

>Subversion Revision: 239107
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 79cbefc1b389b5c407c73dc15394980702f553f8..08b124d2a1c3b80243cc9824f8b9de9963f12a7f 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,22 @@
>+2018-12-12  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 NOBODY (OOPS!).
>+
>+        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-09  Darin Adler  <darin@apple.com>
> 
>         [iOS] Zero memory containing the password for PDF documents when a WKPDFView is deallocated
>diff --git a/Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp b/Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp
>index acb99fc56a94f18dd8a41f83465a1b88279b03d2..ad21dd2fe26eb23c869d05b81fed33491bf6fc8b 100644
>--- a/Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp
>+++ b/Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp
>@@ -52,6 +52,11 @@ UserMediaPermissionRequestManager::UserMediaPermissionRequestManager(WebPage& pa
> }
> 
> UserMediaPermissionRequestManager::~UserMediaPermissionRequestManager()
>+{
>+    clear();
>+}
>+
>+void UserMediaPermissionRequestManager::clear()
> {
>     for (auto& sandboxExtension : m_userMediaDeviceSandboxExtensions)
>         sandboxExtension.value->revoke();
>diff --git a/Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h b/Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h
>index 6004290399e02f4911e55502d073bfb4f3608062..851b7eef7bfae58fc1752ce2f31f0b6ca212d20c 100644
>--- a/Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h
>+++ b/Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h
>@@ -57,6 +57,7 @@ public:
>     void removeDeviceChangeObserver(WebCore::UserMediaClient::DeviceChangeObserverToken);
> 
>     void captureDevicesChanged();
>+    void clear();
> 
> private:
>     void sendUserMediaRequest(WebCore::UserMediaRequest&);
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>index 99a069c55c78fb959ae4535a633a089b9e0de3cf..cb719d405c43adaee4f2c538afaa821b1abdc61c 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>@@ -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)
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.h b/Source/WebKit/WebProcess/WebPage/WebPage.h
>index d4a9b7d49cd9480f02ca5850715286b54ea3121b..e0af2f636866d4beb4a9c5e9e0ba1d0684871004 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.h
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.h
>@@ -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;
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index b2bfc801513e77db963f766ce53f05929f1ec56a..2b15843c97bc676da13ba0a7f32bbe8633884a68 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-12-12  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 NOBODY (OOPS!).
>+
>+        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):
>+
> 2018-12-12  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r239103.
>diff --git a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>index a89e05b7d8f14e6d4376037c5e4d25d3f2118d32..7e8ed3be51484308e0d8a1a9b78ba358eff554a5 100644
>--- a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>+++ b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>@@ -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 */; };
>@@ -1178,6 +1179,7 @@
> 				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 */,
>@@ -1509,6 +1511,7 @@
> 		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>"; };
>@@ -3265,6 +3268,7 @@
> 				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 */,
>@@ -3921,7 +3925,6 @@
> 				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 */,
>@@ -4099,6 +4102,7 @@
> 				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 */,
>@@ -4291,10 +4295,10 @@
> 				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 */,
>diff --git a/Tools/TestWebKitAPI/Tests/WebKit/UserMedia.cpp b/Tools/TestWebKitAPI/Tests/WebKit/UserMedia.cpp
>index 363b753381f48ecbd1612abe84881ca6b27f31ae..2f0cbe203b92d881dc1855069d30e70ccdd321cf 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKit/UserMedia.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WebKit/UserMedia.cpp
>@@ -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)

Actions: View | Formatted Diff | Diff

Attachments on bug 192657: 357209 | 357230