Changeset 241306 in webkit


Ignore:
Timestamp:
Feb 12, 2019 11:29:21 AM (5 years ago)
Author:
achristensen@apple.com
Message:

WebPage::close needs to remove all message receivers associated with that WebPage, not WebPage::~WebPage
https://bugs.webkit.org/show_bug.cgi?id=194522
<rdar://problem/47789393>

Reviewed by Chris Dumez.

Source/WebKit:

The InjectedBundle SPI can retain the WebPage or wrapping objects (WKWebProcessPlugInBrowserContextController/WKBundlePageRef).
This can make it so WebPage::close is called before WebPage::~WebPage, and if the SuspendedPageProxy is reused for a subsequent
navigation to the same domain, the WebProcess is reused with a different WebPage instance with the same PageID, which causes problems
when another WebPage registers message handlers and then the previous WebPage is destroyed, which removes both message handlers.

  • WebProcess/WebPage/WebPage.cpp:

(WebKit::WebPage::~WebPage):
(WebKit::WebPage::close):
(WebKit::WebPage::mainFrameDidLayout):

  • WebProcess/WebPage/WebPage.h:
  • WebProcess/WebProcess.h:

(WebKit::WebProcess::eventDispatcher):

Tools:

  • TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
  • TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm: Added.

(-[BundleRetainPagePlugIn webProcessPlugIn:didCreateBrowserContextController:]):

  • TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
Location:
trunk
Files:
1 added
7 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebKit/ChangeLog

    r241304 r241306  
     12019-02-12  Alex Christensen  <achristensen@webkit.org>
     2
     3        WebPage::close needs to remove all message receivers associated with that WebPage, not WebPage::~WebPage
     4        https://bugs.webkit.org/show_bug.cgi?id=194522
     5        <rdar://problem/47789393>
     6
     7        Reviewed by Chris Dumez.
     8
     9        The InjectedBundle SPI can retain the WebPage or wrapping objects (WKWebProcessPlugInBrowserContextController/WKBundlePageRef).
     10        This can make it so WebPage::close is called before WebPage::~WebPage, and if the SuspendedPageProxy is reused for a subsequent
     11        navigation to the same domain, the WebProcess is reused with a different WebPage instance with the same PageID, which causes problems
     12        when another WebPage registers message handlers and then the previous WebPage is destroyed, which removes both message handlers.
     13
     14        * WebProcess/WebPage/WebPage.cpp:
     15        (WebKit::WebPage::~WebPage):
     16        (WebKit::WebPage::close):
     17        (WebKit::WebPage::mainFrameDidLayout):
     18        * WebProcess/WebPage/WebPage.h:
     19        * WebProcess/WebProcess.h:
     20        (WebKit::WebProcess::eventDispatcher):
     21
    1222019-02-12  Michael Catanzaro  <mcatanzaro@igalia.com>
    223
  • trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp

    r241281 r241306  
    365365    , m_layerHostingMode(parameters.layerHostingMode)
    366366#if PLATFORM(COCOA) || PLATFORM(GTK)
    367     , m_viewGestureGeometryCollector(makeUniqueRef<ViewGestureGeometryCollector>(*this))
     367    , m_viewGestureGeometryCollector(std::make_unique<ViewGestureGeometryCollector>(*this))
    368368#elif HAVE(ACCESSIBILITY) && PLATFORM(GTK)
    369369    , m_accessibilityObject(nullptr)
     
    751751    ASSERT(!m_page);
    752752
    753     auto& webProcess = WebProcess::singleton();
    754 #if ENABLE(ASYNC_SCROLLING)
    755     if (m_useAsyncScrolling)
    756         webProcess.eventDispatcher().removeScrollingTreeForPage(this);
    757 #endif
    758 
    759753    platformDetach();
    760754   
     
    770764        m_footerBanner->detachFromPage();
    771765#endif // !PLATFORM(IOS_FAMILY)
    772 
    773     webProcess.removeMessageReceiver(Messages::WebPage::messageReceiverName(), m_pageID);
    774 
    775     // FIXME: This should be done in the object destructors, and the objects themselves should be message receivers.
    776     webProcess.removeMessageReceiver(Messages::WebInspector::messageReceiverName(), m_pageID);
    777     webProcess.removeMessageReceiver(Messages::WebInspectorUI::messageReceiverName(), m_pageID);
    778     webProcess.removeMessageReceiver(Messages::RemoteWebInspectorUI::messageReceiverName(), m_pageID);
    779 #if ENABLE(FULLSCREEN_API)
    780     webProcess.removeMessageReceiver(Messages::WebFullScreenManager::messageReceiverName(), m_pageID);
    781 #endif
    782766
    783767#ifndef NDEBUG
     
    13191303    m_isRunningModal = false;
    13201304
     1305    auto& webProcess = WebProcess::singleton();
     1306#if ENABLE(ASYNC_SCROLLING)
     1307    if (m_useAsyncScrolling)
     1308        webProcess.eventDispatcher().removeScrollingTreeForPage(this);
     1309#endif
     1310    webProcess.removeMessageReceiver(Messages::WebPage::messageReceiverName(), m_pageID);
     1311    // FIXME: This should be done in the object destructors, and the objects themselves should be message receivers.
     1312    webProcess.removeMessageReceiver(Messages::WebInspector::messageReceiverName(), m_pageID);
     1313    webProcess.removeMessageReceiver(Messages::WebInspectorUI::messageReceiverName(), m_pageID);
     1314    webProcess.removeMessageReceiver(Messages::RemoteWebInspectorUI::messageReceiverName(), m_pageID);
     1315#if ENABLE(FULLSCREEN_API)
     1316    webProcess.removeMessageReceiver(Messages::WebFullScreenManager::messageReceiverName(), m_pageID);
     1317#endif
     1318#if PLATFORM(COCOA) || PLATFORM(GTK)
     1319    m_viewGestureGeometryCollector = nullptr;
     1320#endif
     1321
    13211322    // The WebPage can be destroyed by this call.
    13221323    WebProcess::singleton().removeWebPage(m_pageID);
     
    42174218
    42184219#if PLATFORM(COCOA) || PLATFORM(GTK)
    4219     m_viewGestureGeometryCollector->mainFrameDidLayout();
     4220    if (m_viewGestureGeometryCollector)
     4221        m_viewGestureGeometryCollector->mainFrameDidLayout();
    42204222#endif
    42214223#if PLATFORM(IOS_FAMILY)
  • trunk/Source/WebKit/WebProcess/WebPage/WebPage.h

    r241282 r241306  
    15861586
    15871587#if PLATFORM(COCOA) || PLATFORM(GTK)
    1588     UniqueRef<ViewGestureGeometryCollector> m_viewGestureGeometryCollector;
     1588    std::unique_ptr<ViewGestureGeometryCollector> m_viewGestureGeometryCollector;
    15891589#endif
    15901590
  • trunk/Source/WebKit/WebProcess/WebProcess.h

    r240683 r241306  
    166166#endif
    167167
    168     EventDispatcher& eventDispatcher() { return *m_eventDispatcher; }
     168    EventDispatcher& eventDispatcher() { return m_eventDispatcher.get(); }
    169169
    170170    NetworkProcessConnection& ensureNetworkProcessConnection();
     
    403403    RefPtr<InjectedBundle> m_injectedBundle;
    404404
    405     RefPtr<EventDispatcher> m_eventDispatcher;
     405    Ref<EventDispatcher> m_eventDispatcher;
    406406#if PLATFORM(IOS_FAMILY)
    407407    RefPtr<ViewUpdateDispatcher> m_viewUpdateDispatcher;
  • trunk/Tools/ChangeLog

    r241299 r241306  
     12019-02-12  Alex Christensen  <achristensen@webkit.org>
     2
     3        WebPage::close needs to remove all message receivers associated with that WebPage, not WebPage::~WebPage
     4        https://bugs.webkit.org/show_bug.cgi?id=194522
     5        <rdar://problem/47789393>
     6
     7        Reviewed by Chris Dumez.
     8
     9        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
     10        * TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm: Added.
     11        (-[BundleRetainPagePlugIn webProcessPlugIn:didCreateBrowserContextController:]):
     12        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
     13
    1142019-02-12  Andy Estes  <aestes@apple.com>
    215
  • trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj

    r241134 r241306  
    308308                5C69BDD51F82A7EF000F4F4B /* JavaScriptDuringNavigation.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C69BDD41F82A7EB000F4F4B /* JavaScriptDuringNavigation.mm */; };
    309309                5C7148952123A40A00FDE3C5 /* WKWebsiteDatastore.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C7148942123A40700FDE3C5 /* WKWebsiteDatastore.mm */; };
     310                5C75716122124C5200B9E5AC /* BundleRetainPagePlugIn.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C75715F221249BD00B9E5AC /* BundleRetainPagePlugIn.mm */; };
    310311                5C7964101EB0278D0075D74C /* EventModifiers.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 5C79640F1EB0269B0075D74C /* EventModifiers.cpp */; };
    311312                5C7C74CB1FB529BA002F9ABE /* WebViewScheduleInRunLoop.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C7C74CA1FB528D4002F9ABE /* WebViewScheduleInRunLoop.mm */; };
     
    16911692                5C69BDD41F82A7EB000F4F4B /* JavaScriptDuringNavigation.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = JavaScriptDuringNavigation.mm; sourceTree = "<group>"; };
    16921693                5C7148942123A40700FDE3C5 /* WKWebsiteDatastore.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = WKWebsiteDatastore.mm; sourceTree = "<group>"; };
     1694                5C75715F221249BD00B9E5AC /* BundleRetainPagePlugIn.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = BundleRetainPagePlugIn.mm; sourceTree = "<group>"; };
    16931695                5C79640F1EB0269B0075D74C /* EventModifiers.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = EventModifiers.cpp; sourceTree = "<group>"; };
    16941696                5C7C74CA1FB528D4002F9ABE /* WebViewScheduleInRunLoop.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = WebViewScheduleInRunLoop.mm; sourceTree = "<group>"; };
     
    24682470                                37A709AA1E3EA79000CA5969 /* BundleRangeHandlePlugIn.mm */,
    24692471                                37A709AC1E3EA7E800CA5969 /* BundleRangeHandleProtocol.h */,
     2472                                5C75715F221249BD00B9E5AC /* BundleRetainPagePlugIn.mm */,
    24702473                                1C2B817E1C891E4200A5529F /* CancelFontSubresource.mm */,
    24712474                                1C2B81811C891EFA00A5529F /* CancelFontSubresourcePlugIn.mm */,
     
    43904393                                A13EBBB01B87436F00097110 /* BundleParametersPlugIn.mm in Sources */,
    43914394                                37A709AF1E3EA97E00CA5969 /* BundleRangeHandlePlugIn.mm in Sources */,
     4395                                5C75716122124C5200B9E5AC /* BundleRetainPagePlugIn.mm in Sources */,
    43924396                                1C2B81831C891F0900A5529F /* CancelFontSubresourcePlugIn.mm in Sources */,
    43934397                                5CB18BA81F5645E300EE23C4 /* ClickAutoFillButton.mm in Sources */,
  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

    r240928 r241306  
    24052405)PSONRESOURCE";
    24062406
    2407 TEST(ProcessSwap, ReuseSuspendedProcessForRegularNavigation)
     2407enum class RetainPageInBundle { No, Yes };
     2408
     2409void testReuseSuspendedProcessForRegularNavigation(RetainPageInBundle retainPageInBundle)
    24082410{
    24092411    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
    24102412    [processPoolConfiguration setProcessSwapsOnNavigation:YES];
    2411     auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
     2413    if (retainPageInBundle == RetainPageInBundle::Yes)
     2414        [processPoolConfiguration setInjectedBundleURL:[[NSBundle mainBundle] URLForResource:@"TestWebKitAPI" withExtension:@"wkbundle"]];
     2415
     2416    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
     2417    if (retainPageInBundle == RetainPageInBundle::Yes)
     2418        [processPool _setObject:@"BundleRetainPagePlugIn" forBundleParameter:TestWebKitAPI::Util::TestPlugInClassNameParameter];
    24122419
    24132420    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
     
    24452452
    24462453    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
     2454}
     2455
     2456TEST(ProcessSwap, ReuseSuspendedProcessForRegularNavigationRetainBundlePage)
     2457{
     2458    testReuseSuspendedProcessForRegularNavigation(RetainPageInBundle::Yes);
     2459}
     2460
     2461TEST(ProcessSwap, ReuseSuspendedProcessForRegularNavigation)
     2462{
     2463    testReuseSuspendedProcessForRegularNavigation(RetainPageInBundle::No);
    24472464}
    24482465
Note: See TracChangeset for help on using the changeset viewer.