[WK2] API::Navigation objects are leaked on history navigation to HistoryItems in...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 14 Jun 2015 04:53:14 +0000 (04:53 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 14 Jun 2015 04:53:14 +0000 (04:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145948

Reviewed by Darin Adler.

Source/WebCore:

API::Navigation objects were leaked on history navigation to
HistoryItems in PageCache. In such case, we would create 2 Navigation
objects instead of 1 and the first one would be leaked. The reason
we create the second one is because we fail to pass along the
navigationID from the UIProcess to the WebProcess and then back to the
UIProcess. On the IPC back to the UIProcess, the navigationID ends up
being 0 so the UIProcess creates a new Navigation object, thinking that
the load was triggered by the WebContent process.

We now pass along the navigationID, even if the HistoryItem is in the
PageCache and we end up reusing the cached DocumentLoader, instead of
creating a new one. A new updateCachedDocumentLoader() delegate is
added to the FrameLoaderClient, similarly to the pre-existing
createDocumentLoader() but for the case where the DocumentLoader gets
reused.

* loader/EmptyClients.h:
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadDifferentDocumentItem):
* loader/FrameLoaderClient.h:

Source/WebKit/mac:

Add empty implementation for new
FrameLoaderClient::updatedCachedDocumentLoader().

* WebCoreSupport/WebFrameLoaderClient.h:

Source/WebKit/win:

Add empty implementation for new
FrameLoaderClient::updatedCachedDocumentLoader().

* WebCoreSupport/WebFrameLoaderClient.h:

Source/WebKit2:

API::Navigation objects were leaked on history navigation to
HistoryItems in PageCache. In such case, we would create 2 Navigation
objects instead of 1 and the first one would be leaked. The reason
we create the second one is because we fail to pass along the
navigationID from the UIProcess to the WebProcess and then back to the
UIProcess. On the IPC back to the UIProcess, the navigationID ends up
being 0 so the UIProcess creates a new Navigation object, thinking that
the load was triggered by the WebContent process.

We now pass along the navigationID, even if the HistoryItem is in the
PageCache and we end up reusing the cached DocumentLoader, instead of
creating a new one. A new updateCachedDocumentLoader() delegate is
added to the FrameLoaderClient, similarly to the pre-existing
createDocumentLoader() but for the case where the DocumentLoader gets
reused.

* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::updateCachedDocumentLoader):
* WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::goForward):
(WebKit::WebPage::goBack):
(WebKit::WebPage::goToBackForwardItem):
(WebKit::WebPage::updateCachedDocumentLoader):
* WebProcess/WebPage/WebPage.h:

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

13 files changed:
Source/WebCore/ChangeLog
Source/WebCore/loader/EmptyClients.h
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoaderClient.h
Source/WebKit/mac/ChangeLog
Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.h
Source/WebKit/win/ChangeLog
Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.h
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h
Source/WebKit2/WebProcess/WebPage/WebPage.cpp
Source/WebKit2/WebProcess/WebPage/WebPage.h

index d82fed8..adecc9b 100644 (file)
@@ -1,3 +1,31 @@
+2015-06-13  Chris Dumez  <cdumez@apple.com>
+
+        [WK2] API::Navigation objects are leaked on history navigation to HistoryItems in PageCache
+        https://bugs.webkit.org/show_bug.cgi?id=145948
+
+        Reviewed by Darin Adler.
+
+        API::Navigation objects were leaked on history navigation to
+        HistoryItems in PageCache. In such case, we would create 2 Navigation
+        objects instead of 1 and the first one would be leaked. The reason
+        we create the second one is because we fail to pass along the
+        navigationID from the UIProcess to the WebProcess and then back to the
+        UIProcess. On the IPC back to the UIProcess, the navigationID ends up
+        being 0 so the UIProcess creates a new Navigation object, thinking that
+        the load was triggered by the WebContent process.
+
+        We now pass along the navigationID, even if the HistoryItem is in the
+        PageCache and we end up reusing the cached DocumentLoader, instead of
+        creating a new one. A new updateCachedDocumentLoader() delegate is
+        added to the FrameLoaderClient, similarly to the pre-existing
+        createDocumentLoader() but for the case where the DocumentLoader gets
+        reused.
+
+        * loader/EmptyClients.h:
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadDifferentDocumentItem):
+        * loader/FrameLoaderClient.h:
+
 2015-06-13  Xabier Rodriguez Calvar  <calvaris@igalia.com> and Youenn Fablet <youenn.fablet@crf.canon.fr>
 
         [Streams API] ReadableJSStream should handle promises returned by JS source start callback
 2015-06-13  Xabier Rodriguez Calvar  <calvaris@igalia.com> and Youenn Fablet <youenn.fablet@crf.canon.fr>
 
         [Streams API] ReadableJSStream should handle promises returned by JS source start callback
index ddee4a6..6048ffb 100644 (file)
@@ -347,6 +347,7 @@ public:
     virtual void prepareForDataSourceReplacement() override { }
 
     virtual PassRefPtr<DocumentLoader> createDocumentLoader(const ResourceRequest&, const SubstituteData&) override;
     virtual void prepareForDataSourceReplacement() override { }
 
     virtual PassRefPtr<DocumentLoader> createDocumentLoader(const ResourceRequest&, const SubstituteData&) override;
+    virtual void updateCachedDocumentLoader(DocumentLoader&) override { }
     virtual void setTitle(const StringWithDirection&, const URL&) override { }
 
     virtual String userAgent(const URL&) override { return ""; }
     virtual void setTitle(const StringWithDirection&, const URL&) override { }
 
     virtual String userAgent(const URL&) override { return ""; }
index 628e67d..2ef9500 100644 (file)
@@ -3177,6 +3177,7 @@ void FrameLoader::loadDifferentDocumentItem(HistoryItem& item, FrameLoadType loa
 
     if (CachedPage* cachedPage = PageCache::singleton().get(item, m_frame.page())) {
         auto documentLoader = cachedPage->documentLoader();
 
     if (CachedPage* cachedPage = PageCache::singleton().get(item, m_frame.page())) {
         auto documentLoader = cachedPage->documentLoader();
+        m_client.updateCachedDocumentLoader(*documentLoader);
         documentLoader->setTriggeringAction(NavigationAction(documentLoader->request(), loadType, false));
         documentLoader->setLastCheckedRequest(ResourceRequest());
         loadWithDocumentLoader(documentLoader, loadType, 0, AllowNavigationToInvalidURL::Yes);
         documentLoader->setTriggeringAction(NavigationAction(documentLoader->request(), loadType, false));
         documentLoader->setLastCheckedRequest(ResourceRequest());
         loadWithDocumentLoader(documentLoader, loadType, 0, AllowNavigationToInvalidURL::Yes);
index afaba6a..c13a196 100644 (file)
@@ -251,6 +251,7 @@ namespace WebCore {
         virtual void prepareForDataSourceReplacement() = 0;
 
         virtual PassRefPtr<DocumentLoader> createDocumentLoader(const ResourceRequest&, const SubstituteData&) = 0;
         virtual void prepareForDataSourceReplacement() = 0;
 
         virtual PassRefPtr<DocumentLoader> createDocumentLoader(const ResourceRequest&, const SubstituteData&) = 0;
+        virtual void updateCachedDocumentLoader(DocumentLoader&) = 0;
         virtual void setTitle(const StringWithDirection&, const URL&) = 0;
 
         virtual String userAgent(const URL&) = 0;
         virtual void setTitle(const StringWithDirection&, const URL&) = 0;
 
         virtual String userAgent(const URL&) = 0;
index c7cc724..cbb741b 100644 (file)
@@ -1,3 +1,15 @@
+2015-06-13  Chris Dumez  <cdumez@apple.com>
+
+        [WK2] API::Navigation objects are leaked on history navigation to HistoryItems in PageCache
+        https://bugs.webkit.org/show_bug.cgi?id=145948
+
+        Reviewed by Darin Adler.
+
+        Add empty implementation for new
+        FrameLoaderClient::updatedCachedDocumentLoader().
+
+        * WebCoreSupport/WebFrameLoaderClient.h:
+
 2015-06-11  Mark Lam  <mark.lam@apple.com>
 
         WebCore::reportException() needs to be able to accept a raw thrown value in addition to Exception objects.
 2015-06-11  Mark Lam  <mark.lam@apple.com>
 
         WebCore::reportException() needs to be able to accept a raw thrown value in addition to Exception objects.
index f6f94a2..7cf158f 100644 (file)
@@ -193,6 +193,7 @@ private:
     virtual void didFinishLoad() override;
     virtual void prepareForDataSourceReplacement() override;
     virtual PassRefPtr<WebCore::DocumentLoader> createDocumentLoader(const WebCore::ResourceRequest&, const WebCore::SubstituteData&) override;
     virtual void didFinishLoad() override;
     virtual void prepareForDataSourceReplacement() override;
     virtual PassRefPtr<WebCore::DocumentLoader> createDocumentLoader(const WebCore::ResourceRequest&, const WebCore::SubstituteData&) override;
+    virtual void updateCachedDocumentLoader(WebCore::DocumentLoader&) override { }
 
     virtual void setTitle(const WebCore::StringWithDirection&, const WebCore::URL&) override;
 
 
     virtual void setTitle(const WebCore::StringWithDirection&, const WebCore::URL&) override;
 
index 4e185a9..b03e6fc 100644 (file)
@@ -1,3 +1,15 @@
+2015-06-13  Chris Dumez  <cdumez@apple.com>
+
+        [WK2] API::Navigation objects are leaked on history navigation to HistoryItems in PageCache
+        https://bugs.webkit.org/show_bug.cgi?id=145948
+
+        Reviewed by Darin Adler.
+
+        Add empty implementation for new
+        FrameLoaderClient::updatedCachedDocumentLoader().
+
+        * WebCoreSupport/WebFrameLoaderClient.h:
+
 2015-06-11  Mark Lam  <mark.lam@apple.com>
 
         WebCore::reportException() needs to be able to accept a raw thrown value in addition to Exception objects.
 2015-06-11  Mark Lam  <mark.lam@apple.com>
 
         WebCore::reportException() needs to be able to accept a raw thrown value in addition to Exception objects.
index c4acfc5..87b6d51 100644 (file)
@@ -152,6 +152,8 @@ public:
     virtual WTF::String userAgent(const WebCore::URL&) override;
 
     virtual PassRefPtr<WebCore::DocumentLoader> createDocumentLoader(const WebCore::ResourceRequest&, const WebCore::SubstituteData&);
     virtual WTF::String userAgent(const WebCore::URL&) override;
 
     virtual PassRefPtr<WebCore::DocumentLoader> createDocumentLoader(const WebCore::ResourceRequest&, const WebCore::SubstituteData&);
+    virtual void updateCachedDocumentLoader(WebCore::DocumentLoader&) override { }
+
     virtual void setTitle(const WebCore::StringWithDirection&, const WebCore::URL&);
 
     virtual void savePlatformDataToCachedFrame(WebCore::CachedFrame*) override;
     virtual void setTitle(const WebCore::StringWithDirection&, const WebCore::URL&);
 
     virtual void savePlatformDataToCachedFrame(WebCore::CachedFrame*) override;
index e997c6d..3f942d8 100644 (file)
@@ -1,3 +1,36 @@
+2015-06-13  Chris Dumez  <cdumez@apple.com>
+
+        [WK2] API::Navigation objects are leaked on history navigation to HistoryItems in PageCache
+        https://bugs.webkit.org/show_bug.cgi?id=145948
+
+        Reviewed by Darin Adler.
+
+        API::Navigation objects were leaked on history navigation to
+        HistoryItems in PageCache. In such case, we would create 2 Navigation
+        objects instead of 1 and the first one would be leaked. The reason
+        we create the second one is because we fail to pass along the
+        navigationID from the UIProcess to the WebProcess and then back to the
+        UIProcess. On the IPC back to the UIProcess, the navigationID ends up
+        being 0 so the UIProcess creates a new Navigation object, thinking that
+        the load was triggered by the WebContent process.
+
+        We now pass along the navigationID, even if the HistoryItem is in the
+        PageCache and we end up reusing the cached DocumentLoader, instead of
+        creating a new one. A new updateCachedDocumentLoader() delegate is
+        added to the FrameLoaderClient, similarly to the pre-existing
+        createDocumentLoader() but for the case where the DocumentLoader gets
+        reused.
+
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::updateCachedDocumentLoader):
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::goForward):
+        (WebKit::WebPage::goBack):
+        (WebKit::WebPage::goToBackForwardItem):
+        (WebKit::WebPage::updateCachedDocumentLoader):
+        * WebProcess/WebPage/WebPage.h:
+
 2015-06-12  Gavin Barraclough  <barraclough@apple.com>
 
         Add private API to force page to always run at foreground priority
 2015-06-12  Gavin Barraclough  <barraclough@apple.com>
 
         Add private API to force page to always run at foreground priority
index 61b14ec..829b737 100644 (file)
@@ -1246,6 +1246,11 @@ PassRefPtr<DocumentLoader> WebFrameLoaderClient::createDocumentLoader(const Reso
     return m_frame->page()->createDocumentLoader(*m_frame->coreFrame(), request, substituteData);
 }
 
     return m_frame->page()->createDocumentLoader(*m_frame->coreFrame(), request, substituteData);
 }
 
+void WebFrameLoaderClient::updateCachedDocumentLoader(WebCore::DocumentLoader& loader)
+{
+    m_frame->page()->updateCachedDocumentLoader(static_cast<WebDocumentLoader&>(loader), *m_frame->coreFrame());
+}
+
 void WebFrameLoaderClient::setTitle(const StringWithDirection& title, const URL& url)
 {
     WebPage* webPage = m_frame->page();
 void WebFrameLoaderClient::setTitle(const StringWithDirection& title, const URL& url)
 {
     WebPage* webPage = m_frame->page();
index 4665e84..3cb15e9 100644 (file)
@@ -164,6 +164,8 @@ private:
     virtual void prepareForDataSourceReplacement() override;
     
     virtual PassRefPtr<WebCore::DocumentLoader> createDocumentLoader(const WebCore::ResourceRequest&, const WebCore::SubstituteData&) override;
     virtual void prepareForDataSourceReplacement() override;
     
     virtual PassRefPtr<WebCore::DocumentLoader> createDocumentLoader(const WebCore::ResourceRequest&, const WebCore::SubstituteData&) override;
+    virtual void updateCachedDocumentLoader(WebCore::DocumentLoader&) override;
+
     virtual void setTitle(const WebCore::StringWithDirection&, const WebCore::URL&) override;
     
     virtual String userAgent(const WebCore::URL&) override;
     virtual void setTitle(const WebCore::StringWithDirection&, const WebCore::URL&) override;
     
     virtual String userAgent(const WebCore::URL&) override;
index 006a91e..358e1c0 100644 (file)
@@ -1161,8 +1161,7 @@ void WebPage::goForward(uint64_t navigationID, uint64_t backForwardItemID)
         return;
 
     ASSERT(!m_pendingNavigationID);
         return;
 
     ASSERT(!m_pendingNavigationID);
-    if (!item->isInPageCache())
-        m_pendingNavigationID = navigationID;
+    m_pendingNavigationID = navigationID;
 
     m_page->goToItem(*item, FrameLoadType::Forward);
 }
 
     m_page->goToItem(*item, FrameLoadType::Forward);
 }
@@ -1177,8 +1176,7 @@ void WebPage::goBack(uint64_t navigationID, uint64_t backForwardItemID)
         return;
 
     ASSERT(!m_pendingNavigationID);
         return;
 
     ASSERT(!m_pendingNavigationID);
-    if (!item->isInPageCache())
-        m_pendingNavigationID = navigationID;
+    m_pendingNavigationID = navigationID;
 
     m_page->goToItem(*item, FrameLoadType::Back);
 }
 
     m_page->goToItem(*item, FrameLoadType::Back);
 }
@@ -1193,8 +1191,7 @@ void WebPage::goToBackForwardItem(uint64_t navigationID, uint64_t backForwardIte
         return;
 
     ASSERT(!m_pendingNavigationID);
         return;
 
     ASSERT(!m_pendingNavigationID);
-    if (!item->isInPageCache())
-        m_pendingNavigationID = navigationID;
+    m_pendingNavigationID = navigationID;
 
     m_page->goToItem(*item, FrameLoadType::IndexedBackForward);
 }
 
     m_page->goToItem(*item, FrameLoadType::IndexedBackForward);
 }
@@ -4924,6 +4921,14 @@ PassRefPtr<DocumentLoader> WebPage::createDocumentLoader(Frame& frame, const Res
     return documentLoader.release();
 }
 
     return documentLoader.release();
 }
 
+void WebPage::updateCachedDocumentLoader(WebDocumentLoader& documentLoader, Frame& frame)
+{
+    if (m_pendingNavigationID && frame.isMainFrame()) {
+        documentLoader.setNavigationID(m_pendingNavigationID);
+        m_pendingNavigationID = 0;
+    }
+}
+
 void WebPage::getBytecodeProfile(uint64_t callbackID)
 {
     if (!JSDOMWindow::commonVM().m_perBytecodeProfiler) {
 void WebPage::getBytecodeProfile(uint64_t callbackID)
 {
     if (!JSDOMWindow::commonVM().m_perBytecodeProfiler) {
index 018281c..f9e6136 100644 (file)
@@ -153,6 +153,7 @@ class VisibleContentRectUpdateInfo;
 class WebColorChooser;
 class WebContextMenu;
 class WebContextMenuItemData;
 class WebColorChooser;
 class WebContextMenu;
 class WebContextMenuItemData;
+class WebDocumentLoader;
 class WebEvent;
 class WebFrame;
 class WebFullScreenManager;
 class WebEvent;
 class WebFrame;
 class WebFullScreenManager;
@@ -871,6 +872,7 @@ public:
     void setScrollbarOverlayStyle(WTF::Optional<uint32_t /* WebCore::ScrollbarOverlayStyle */> scrollbarStyle);
 
     PassRefPtr<WebCore::DocumentLoader> createDocumentLoader(WebCore::Frame&, const WebCore::ResourceRequest&, const WebCore::SubstituteData&);
     void setScrollbarOverlayStyle(WTF::Optional<uint32_t /* WebCore::ScrollbarOverlayStyle */> scrollbarStyle);
 
     PassRefPtr<WebCore::DocumentLoader> createDocumentLoader(WebCore::Frame&, const WebCore::ResourceRequest&, const WebCore::SubstituteData&);
+    void updateCachedDocumentLoader(WebDocumentLoader&, WebCore::Frame&);
 
     void getBytecodeProfile(uint64_t callbackID);
     
 
     void getBytecodeProfile(uint64_t callbackID);