Attachment 346559 Details for Bug 188321 – Patch

[patch] Patch

bug-188321-20180803162507.patch (text/plain), 6.28 KB, created by Chris Dumez on 2018-08-03 16:25:07 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-08-03 16:25:07 PDT

Size: 6.28 KB

patch

obsolete

>Subversion Revision: 234545
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 0e9e004457249385d6dd2399c8be90e3e21e8095..5a62568ff50df062c68de6ea0c47c1b065026b1e 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,59 @@
>+2018-08-03  Chris Dumez  <cdumez@apple.com>
>+
>+        Fix IPC::Connection leak in StorageManager
>+        https://bugs.webkit.org/show_bug.cgi?id=188321
>+        <rdar://problem/42748485>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When a StorageMap is destroyed on WebContent process side, StorageManager::destroyStorageMap()
>+        gets called via IPC with a (IPC::Connection, StorageMapID) pair. Normally, it removes this
>+        pair from m_storageAreasByConnection. However, if this is a *transient* StorageMap (sessionStorage),
>+        then we keep the pair in the map and we merely remove the StorageMapID as a listener from the
>+        StorageArea. We do this so that:
>+        1. The StorageArea stays alive so that it can be reused later on for the same security origin, on
>+           the same IPC::Connection (logic for this is in StorageManager::createTransientLocalStorageMap()
>+        2. Removing the StorageMapID as a listener from the StorageArea is important because
>+           StorageArea::m_eventListeners holds a strong reference to the IPC::Connection in a std::pair
>+           with the StorageMapID (HashSet<std::pair<RefPtr<IPC::Connection>, uint64_t>> m_eventListeners).
>+
>+        As mentioned in 1 above, in StorageManager::createTransientLocalStorageMap(), there is logic to
>+        check if there is already an existing StorageArea for the given IPC::Connection that is transient
>+        and is for the same security origin. In this case, we could avoid constructing a new StorageArea
>+        and we would:
>+        1. Add a new entry to m_storageAreasByConnection with the key (connection, newStorageMapID), using
>+           same same StorageArea as value.
>+        2. Remove the previous (connection, oldStorageMapID) key from m_storageAreasByConnection.
>+
>+        Step 2 here is wrong and is updated in this patch. It is only safe to remove the previous 
>+        (connection, oldStorageMapID) if this oldStorageMapID no longer exists (i.e. destroyStorageMap()
>+        was already called for it). This patch thus adds a check before removing (connection, oldStorageMapID)
>+        from the HashMap to make sure that the oldStorageMapID is no longer a listener of the StorageArea).
>+
>+        This would cause leaks in the following case:
>+        1. We construct a StorageArea for (connection1, storageMapId1)
>+        2. We ask for a StorageArea for (connection1, storageMapId2) and decide to reuse the existing StorageArea
>+           since it has the same SecurityOrigin.
>+        3. As a result of step2, we would remove (connection1, storageMapId1) from m_storageAreasByConnection
>+           and add (connection1, storageMapId2), even though there is still a StorageMap with storageMapId1
>+           on WebContent process side.
>+        4. Later on, we would try to call destroyStorageMap(connection1, storageMap1), it would fail to find
>+           it in m_storageAreasByConnection and return early. It would therefore fail to remove storageMapId1
>+           as a listener of the StorageArea which still exists.
>+        -> This would leak the IPC::Connection that there would be a std::pair<RefPtr<IPC::Connection>, StorageMapID>
>+           with value (connection1, storageMap1) which would get leaked and it would ref the IPC::Connection.
>+
>+        This code should really be refactored to be less leak prone but I have kept the patch minimal for now
>+        to facilitate cherry-picking.
>+
>+        Note that this would reproduce very easily on sina.com.cn, when clicking bold links at the top, which
>+        opens new tabs to different pages in the same WebContent process. When closing all Safari windows, the
>+        IPC::Connection for this WebContent process would stay alive.
>+
>+        * UIProcess/WebStorage/StorageManager.cpp:
>+        (WebKit::StorageManager::StorageArea::hasListener const):
>+        (WebKit::StorageManager::createTransientLocalStorageMap):
>+
> 2018-08-03  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r234513.
>diff --git a/Source/WebKit/UIProcess/WebStorage/StorageManager.cpp b/Source/WebKit/UIProcess/WebStorage/StorageManager.cpp
>index 9583175e394f339535bc642220374a7631f0e719..c76b73af0e284a457ef4f84d613a3378c8b536a6 100644
>--- a/Source/WebKit/UIProcess/WebStorage/StorageManager.cpp
>+++ b/Source/WebKit/UIProcess/WebStorage/StorageManager.cpp
>@@ -51,6 +51,7 @@ public:
> 
>     void addListener(IPC::Connection&, uint64_t storageMapID);
>     void removeListener(IPC::Connection&, uint64_t storageMapID);
>+    bool hasListener(IPC::Connection&, uint64_t storageMapID) const;
> 
>     Ref<StorageArea> clone() const;
> 
>@@ -196,6 +197,11 @@ void StorageManager::StorageArea::removeListener(IPC::Connection& connection, ui
>     m_eventListeners.remove(std::make_pair(&connection, storageMapID));
> }
> 
>+bool StorageManager::StorageArea::hasListener(IPC::Connection& connection, uint64_t storageMapID) const
>+{
>+    return m_eventListeners.contains(std::make_pair(&connection, storageMapID));
>+}
>+
> Ref<StorageManager::StorageArea> StorageManager::StorageArea::clone() const
> {
>     ASSERT(!m_localStorageNamespace);
>@@ -697,7 +703,12 @@ void StorageManager::createTransientLocalStorageMap(IPC::Connection& connection,
>         if (!origin.securityOrigin()->isSameSchemeHostPort(area->securityOrigin().securityOrigin().get()))
>             continue;
>         area->addListener(connection, storageMapID);
>-        m_storageAreasByConnection.remove(it);
>+        // If the storageMapID used as key in m_storageAreasByConnection is no longer one of the StorageArea's listeners, then this means
>+        // that destroyStorageMap() was already called for that storageMapID but it decided not to remove it from m_storageAreasByConnection
>+        // so that we could reuse it later on for the same connection/origin combo. In this case, it is safe to remove the previous
>+        // storageMapID from m_storageAreasByConnection.
>+        if (!area->hasListener(connection, it->key.second))
>+            m_storageAreasByConnection.remove(it);
>         m_storageAreasByConnection.add({ &connection, storageMapID }, WTFMove(area));
>         return;
>     }

Actions: View | Formatted Diff | Diff

Attachments on bug 188321: 346559 | 346582