Changeset 239167 in webkit


Ignore:
Timestamp:
Dec 13, 2018 8:59:15 AM (5 years ago)
Author:
youenn@apple.com
Message:

On page close, WebPage::m_userMediaPermissionRequestManager is nullified too early
​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.
Location:
trunk
Files:
1 added
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r239166 r239167  
     12018-12-13  Youenn Fablet  <youenn@apple.com>
     2
     3        On page close, WebPage::m_userMediaPermissionRequestManager is nullified too early
     4        https://bugs.webkit.org/show_bug.cgi?id=192657
     5
     6        Reviewed by Eric Carlson.
     7
     8        Instead of nullifying the manager, make it a UniqueRef and clear it on closing the page.
     9        This ensures we revoke the sandbox extensions as early as possible and keep the manager lifetime simple.
     10
     11        * WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:
     12        (WebKit::UserMediaPermissionRequestManager::~UserMediaPermissionRequestManager):
     13        (WebKit::UserMediaPermissionRequestManager::clear):
     14        * WebProcess/MediaStream/UserMediaPermissionRequestManager.h:
     15        * WebProcess/WebPage/WebPage.cpp:
     16        (WebKit::WebPage::close):
     17        * WebProcess/WebPage/WebPage.h:
     18        (WebKit::WebPage::userMediaPermissionRequestManager):
     19
    1202018-12-13  Chris Fleizach  <cfleizach@apple.com>
    221

  • trunk/Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp

    r239006 r239167  
    5454UserMediaPermissionRequestManager::~UserMediaPermissionRequestManager()
    5555{
     56    clear();
     57}
     58
     59void UserMediaPermissionRequestManager::clear()
     60{
    5661    for (auto& sandboxExtension : m_userMediaDeviceSandboxExtensions)
    5762        sandboxExtension.value->revoke();

  • trunk/Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h

    r239006 r239167  
    5858
    5959    void captureDevicesChanged();
     60    void clear();
    6061
    6162private:

  • trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp

    r239039 r239167  
    380380#endif
    381381#if ENABLE(MEDIA_STREAM)
    382     , m_userMediaPermissionRequestManager { std::make_unique<UserMediaPermissionRequestManager>(*this) }
     382    , m_userMediaPermissionRequestManager { makeUniqueRef<UserMediaPermissionRequestManager>(*this) }
    383383#endif
    384384    , m_pageScrolledHysteresis([this](PAL::HysteresisState state) { if (state == PAL::HysteresisState::Stopped) pageStoppedScrolling(); }, pageScrollHysteresisDuration)
    … …  
    12221222
    12231223#if ENABLE(MEDIA_STREAM)
    1224     m_userMediaPermissionRequestManager = nullptr;
     1224    m_userMediaPermissionRequestManager->clear();
    12251225#endif
    12261226

  • trunk/Source/WebKit/WebProcess/WebPage/WebPage.h

    r239039 r239167  
    577577
    578578#if ENABLE(MEDIA_STREAM)
    579     UserMediaPermissionRequestManager& userMediaPermissionRequestManager() { return *m_userMediaPermissionRequestManager; }
     579    UserMediaPermissionRequestManager& userMediaPermissionRequestManager() { return m_userMediaPermissionRequestManager; }
    580580    void prepareToSendUserMediaPermissionRequest();
    581581    void captureDevicesChanged();
    … …  
    16271627
    16281628#if ENABLE(MEDIA_STREAM)
    1629     std::unique_ptr<UserMediaPermissionRequestManager> m_userMediaPermissionRequestManager;
     1629    UniqueRef<UserMediaPermissionRequestManager> m_userMediaPermissionRequestManager;
    16301630#endif
    16311631

  • trunk/Tools/ChangeLog

    r239159 r239167  
     12018-12-13  Youenn Fablet  <youenn@apple.com>
     2
     3        On page close, WebPage::m_userMediaPermissionRequestManager is nullified too early
     4        https://bugs.webkit.org/show_bug.cgi?id=192657
     5
     6        Reviewed by Eric Carlson.
     7
     8        Add a test that loads a page registering ondevicechange,
     9        load another page in the same process, closes the first page.
     10        Ensure that the process does not crash in that case.
     11
     12        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
     13        * TestWebKitAPI/Tests/WebKit/UserMedia.cpp:
     14        (TestWebKitAPI::TEST):
     15        (TestWebKitAPI::didCrashCallback):
     16        * TestWebKitAPI/Tests/WebKit/ondevicechange.html: Added.
     17
    1182018-12-13  Carlos Eduardo Ramalho  <cadubentzen@gmail.com>
    219

  • trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj

    r239077 r239167  
    2727                07492B3C1DF8B86600633DE1 /* enumerateMediaDevices.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 07492B391DF8ADA400633DE1 /* enumerateMediaDevices.html */; };
    2828                074994421EA5034B000DA44E /* getUserMedia.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 4A410F4D19AF7BEF002EBAB5 /* getUserMedia.html */; };
     29                074994421EA5034B000DA44F /* ondevicechange.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 4A410F4D19AF7BEF002EBAB6 /* ondevicechange.html */; };
    2930                076E507F1F4513D6006E9F5A /* Logging.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 076E507E1F45031E006E9F5A /* Logging.cpp */; };
    3031                0799C3491EBA2D7B003B7532 /* UserMediaDisabled.mm in Sources */ = {isa = PBXBuildFile; fileRef = 07EDEFAC1EB9400C00D43292 /* UserMediaDisabled.mm */; };
    … …  
    11791180                                CDB5DFFF213610FA00D3E189 /* now-playing.html in Copy Resources */,
    11801181                                93E2D2761ED7D53200FA76F6 /* offscreen-iframe-of-media-document.html in Copy Resources */,
     1182                                074994421EA5034B000DA44F /* ondevicechange.html in Copy Resources */,
    11811183                                CEA6CF2819CCF69D0064F5A7 /* open-and-close-window.html in Copy Resources */,
    11821184                                7CCB99231D3B4A46003922F6 /* open-multiple-external-url.html in Copy Resources */,
    … …  
    15101512                4A410F4B19AF7BD6002EBAB5 /* UserMedia.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = UserMedia.cpp; sourceTree = "<group>"; };
    15111513                4A410F4D19AF7BEF002EBAB5 /* getUserMedia.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = getUserMedia.html; sourceTree = "<group>"; };
     1514                4A410F4D19AF7BEF002EBAB6 /* ondevicechange.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = ondevicechange.html; sourceTree = "<group>"; };
    15121515                4BB4160116815B2600824238 /* JSWrapperForNodeInWebFrame.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = JSWrapperForNodeInWebFrame.mm; sourceTree = "<group>"; };
    15131516                4BB4160316815F9100824238 /* ElementAtPointInWebFrame.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ElementAtPointInWebFrame.mm; sourceTree = "<group>"; };
    … …  
    32663269                                5797FE321EB15A8900B2F4A0 /* navigation-client-default-crypto.html */,
    32673270                                C99B675E1E39735C00FC6C80 /* no-autoplay-with-controls.html */,
     3271                                4A410F4D19AF7BEF002EBAB6 /* ondevicechange.html */,
    32683272                                CEA6CF2719CCF69D0064F5A7 /* open-and-close-window.html */,
    32693273                                83148B08202AC76800BADE99 /* override-builtins-test.html */,
    … …  
    39223926                                7CCE7EBE1A411A7E00447C4C /* DynamicDeviceScaleFactor.mm in Sources */,
    39233927                                5C0BF8921DD599B600B00328 /* EarlyKVOCrash.mm in Sources */,
    3924                                 52D5D6C021B9F1B30046ABA6 /* RenderingProgress.mm in Sources */,
    39253928                                F44D064A1F3962F2001A0E29 /* EditingTestHarness.mm in Sources */,
    39263929                                7CCE7EE01A411A9A00447C4C /* EditorCommands.mm in Sources */,
    … …  
    41004103                                7CCE7ECA1A411A7E00447C4C /* RenderedImageFromDOMRange.mm in Sources */,
    41014104                                A12DDBFB1E836F0700CF6CAE /* RenderedImageWithOptions.mm in Sources */,
     4105                                52D5D6C021B9F1B30046ABA6 /* RenderingProgress.mm in Sources */,
    41024106                                F464AF9220BB66EA007F9B18 /* RenderingProgressTests.mm in Sources */,
    41034107                                7C83E0C41D0A654200FEBCF3 /* RequiresUserActionForPlayback.mm in Sources */,
    … …  
    42924296                                79C5D431209D768300F1E7CA /* InjectedBundleNodeHandleIsTextField.mm in Sources */,
    42934297                                F44C7A0020F9EEBF0014478C /* ParserYieldTokenPlugIn.mm in Sources */,
    4294                                 5245178721B9F57B0082CB34 /* RenderingProgressPlugIn.mm in Sources */,
    42954298                                A13EBBAB1B87434600097110 /* PlatformUtilitiesCocoa.mm in Sources */,
    42964299                                1A4F81CF1BDFFD53004E672E /* RemoteObjectRegistryPlugIn.mm in Sources */,
    42974300                                A12DDC021E837C2400CF6CAE /* RenderedImageWithOptionsPlugIn.mm in Sources */,
     4301                                5245178721B9F57B0082CB34 /* RenderingProgressPlugIn.mm in Sources */,
    42984302                                7C882E091C80C630006BF731 /* UserContentWorldPlugIn.mm in Sources */,
    42994303                                7C83E03D1D0A60D600FEBCF3 /* UtilitiesCocoa.mm in Sources */,

  • trunk/Tools/TestWebKitAPI/Tests/WebKit/UserMedia.cpp

    r221505 r239167  
    3636namespace TestWebKitAPI {
    3737
     38static bool didCrash;
    3839static bool wasPrompted;
    3940
    … …  
    9697}
    9798
     99static void didCrashCallback(WKPageRef, const void*)
     100{
     101    didCrash = true;
     102    // Set wasPrompted to true to speed things up, we know the test failed.
     103    wasPrompted = true;
     104}
     105
     106TEST(WebKit, OnDeviceChangeCrash)
     107{
     108    auto context = adoptWK(WKContextCreate());
     109
     110    WKRetainPtr<WKPageGroupRef> pageGroup(AdoptWK, WKPageGroupCreateWithIdentifier(Util::toWK("GetUserMedia").get()));
     111    WKPreferencesRef preferences = WKPageGroupGetPreferences(pageGroup.get());
     112    WKPreferencesSetMediaDevicesEnabled(preferences, true);
     113    WKPreferencesSetFileAccessFromFileURLsAllowed(preferences, true);
     114    WKPreferencesSetMediaCaptureRequiresSecureConnection(preferences, false);
     115    WKPreferencesSetMockCaptureDevicesEnabled(preferences, true);
     116
     117    WKPageUIClientV6 uiClient;
     118    memset(&uiClient, 0, sizeof(uiClient));
     119    uiClient.base.version = 6;
     120    uiClient.decidePolicyForUserMediaPermissionRequest = decidePolicyForUserMediaPermissionRequestCallBack;
     121    uiClient.checkUserMediaPermissionForOrigin = checkUserMediaPermissionCallback;
     122
     123    PlatformWebView webView(context.get(), pageGroup.get());
     124    WKPageSetPageUIClient(webView.page(), &uiClient.base);
     125
     126    // Load a page registering ondevicechange handler.
     127    auto url = adoptWK(Util::createURLForResource("ondevicechange", "html"));
     128    ASSERT(url.get());
     129
     130    WKPageLoadURL(webView.page(), url.get());
     131
     132    // Load a second page in same process.
     133    PlatformWebView webView2(context.get(), pageGroup.get());
     134    WKPageSetPageUIClient(webView2.page(), &uiClient.base);
     135    WKPageLoaderClientV0 loaderClient;
     136    memset(&loaderClient, 0, sizeof(loaderClient));
     137    loaderClient.base.version = 0;
     138    loaderClient.processDidCrash = didCrashCallback;
     139    WKPageSetPageLoaderClient(webView2.page(), &loaderClient.base);
     140
     141    wasPrompted = false;
     142    url = adoptWK(Util::createURLForResource("getUserMedia", "html"));
     143    WKPageLoadURL(webView2.page(), url.get());
     144    Util::run(&wasPrompted);
     145    EXPECT_EQ(WKPageGetProcessIdentifier(webView.page()), WKPageGetProcessIdentifier(webView2.page()));
     146
     147    didCrash = false;
     148
     149    // Close first page.
     150    WKPageClose(webView.page());
     151
     152    wasPrompted = false;
     153    url = adoptWK(Util::createURLForResource("getUserMedia", "html"));
     154    WKPageLoadURL(webView2.page(), url.get());
     155    Util::run(&wasPrompted);
     156    // Verify pages process did not crash.
     157    EXPECT_TRUE(!didCrash);
     158}
     159
    98160} // namespace TestWebKitAPI
    99161
Note: See TracChangeset for help on using the changeset viewer.