- Revision
- 293070
- Author
- [email protected]
- Date
- 2022-04-19 22:38:14 -0700 (Tue, 19 Apr 2022)
Log Message
Cherry-pick r291689. rdar://problem/88461772
REGRESSION (iOS 15.1 / r280824) QuickLook - model not loading when passing extra parameters
https://bugs.webkit.org/show_bug.cgi?id=236069
<rdar://problem/88461772>
Reviewed by Darin Adler.
Source/WebCore:
To extend the lifetime of Blob objects while loading/downloading is pending, we rely to
BlobURLHandle objects, which tell the NetworkProcess that we still need this blob data,
even if the corresponding Blob object gets GC'd by JS. The issue here is that the load
was not using the vanilla blob URL but was intead appending a fragment to the URL. As
a result, BlobRegistryImpl::registerBlobURLHandle() would fail to look up the
corresponding blob data and would fail to extend its lifetime.
To address the issue, BlobRegistryImpl::registerBlobURLHandle() / unregisterBlobURLHandle()
now strip fragments from the blob URL, similarly to what was already done inside
BlobRegistryImpl::getBlobDataFromURL().
Test: fast/files/blob-with-fragment-as-frame-url.html
* platform/network/BlobRegistryImpl.cpp:
(WebCore::blobURLWithoutFragment):
(WebCore::BlobRegistryImpl::registerBlobURLHandle):
(WebCore::BlobRegistryImpl::unregisterBlobURLHandle):
Source/WebKit:
Fix theoretical blob data leak. A WebContent process could create several BlobURLHandles for
the same blob URL. This would result in several calls to NetworkConnectionToWebProcess::registerBlobURLHandle()
for the same URL and several calls to to BlobRegistryImpl::registerBlobURLHandle() for the
same URL as well. BlobRegistryImpl is using a HashCountedSet for m_blobReferences in order
to deal with this fact. However, NetworkConnectionToWebProcess was using a simple HashSet
for m_blobURLHandles. As a result, if the WebContent process would exit and didClose()
would get called, the NetworkConnectionToWebProcess may call BlobRegistryImpl::unregisterBlobURLHandle()
only once even though the WebContent process had several handles for this URL, which would
not fully remove the URL from BlobRegistryImpl's HashCountedSet. To address the issue,
NetworkConnectionToWebProcess::m_blobURLHandles is now a HashCountedSet too and we call
BlobRegistryImpl::unregisterBlobURLHandle() as many times as needed in didClose().
* NetworkProcess/NetworkConnectionToWebProcess.cpp:
(WebKit::NetworkConnectionToWebProcess::didClose):
* NetworkProcess/NetworkConnectionToWebProcess.h:
LayoutTests:
Add layout test coverage.
* fast/files/blob-with-fragment-as-frame-url-expected.txt: Added.
* fast/files/blob-with-fragment-as-frame-url.html: Added.
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@291689 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Added Paths
Diff
Modified: branches/safari-613-branch/LayoutTests/ChangeLog (293069 => 293070)
--- branches/safari-613-branch/LayoutTests/ChangeLog 2022-04-20 05:38:09 UTC (rev 293069)
+++ branches/safari-613-branch/LayoutTests/ChangeLog 2022-04-20 05:38:14 UTC (rev 293070)
@@ -1,5 +1,75 @@
2022-04-19 Alan Coon <[email protected]>
+ Cherry-pick r291689. rdar://problem/88461772
+
+ REGRESSION (iOS 15.1 / r280824) QuickLook - model not loading when passing extra parameters
+ https://bugs.webkit.org/show_bug.cgi?id=236069
+ <rdar://problem/88461772>
+
+ Reviewed by Darin Adler.
+
+ Source/WebCore:
+
+ To extend the lifetime of Blob objects while loading/downloading is pending, we rely to
+ BlobURLHandle objects, which tell the NetworkProcess that we still need this blob data,
+ even if the corresponding Blob object gets GC'd by JS. The issue here is that the load
+ was not using the vanilla blob URL but was intead appending a fragment to the URL. As
+ a result, BlobRegistryImpl::registerBlobURLHandle() would fail to look up the
+ corresponding blob data and would fail to extend its lifetime.
+
+ To address the issue, BlobRegistryImpl::registerBlobURLHandle() / unregisterBlobURLHandle()
+ now strip fragments from the blob URL, similarly to what was already done inside
+ BlobRegistryImpl::getBlobDataFromURL().
+
+ Test: fast/files/blob-with-fragment-as-frame-url.html
+
+ * platform/network/BlobRegistryImpl.cpp:
+ (WebCore::blobURLWithoutFragment):
+ (WebCore::BlobRegistryImpl::registerBlobURLHandle):
+ (WebCore::BlobRegistryImpl::unregisterBlobURLHandle):
+
+ Source/WebKit:
+
+ Fix theoretical blob data leak. A WebContent process could create several BlobURLHandles for
+ the same blob URL. This would result in several calls to NetworkConnectionToWebProcess::registerBlobURLHandle()
+ for the same URL and several calls to to BlobRegistryImpl::registerBlobURLHandle() for the
+ same URL as well. BlobRegistryImpl is using a HashCountedSet for m_blobReferences in order
+ to deal with this fact. However, NetworkConnectionToWebProcess was using a simple HashSet
+ for m_blobURLHandles. As a result, if the WebContent process would exit and didClose()
+ would get called, the NetworkConnectionToWebProcess may call BlobRegistryImpl::unregisterBlobURLHandle()
+ only once even though the WebContent process had several handles for this URL, which would
+ not fully remove the URL from BlobRegistryImpl's HashCountedSet. To address the issue,
+ NetworkConnectionToWebProcess::m_blobURLHandles is now a HashCountedSet too and we call
+ BlobRegistryImpl::unregisterBlobURLHandle() as many times as needed in didClose().
+
+ * NetworkProcess/NetworkConnectionToWebProcess.cpp:
+ (WebKit::NetworkConnectionToWebProcess::didClose):
+ * NetworkProcess/NetworkConnectionToWebProcess.h:
+
+ LayoutTests:
+
+ Add layout test coverage.
+
+ * fast/files/blob-with-fragment-as-frame-url-expected.txt: Added.
+ * fast/files/blob-with-fragment-as-frame-url.html: Added.
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@291689 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2022-03-22 Chris Dumez <[email protected]>
+
+ REGRESSION (iOS 15.1 / r280824) QuickLook - model not loading when passing extra parameters
+ https://bugs.webkit.org/show_bug.cgi?id=236069
+ <rdar://problem/88461772>
+
+ Reviewed by Darin Adler.
+
+ Add layout test coverage.
+
+ * fast/files/blob-with-fragment-as-frame-url-expected.txt: Added.
+ * fast/files/blob-with-fragment-as-frame-url.html: Added.
+
+2022-04-19 Alan Coon <[email protected]>
+
Cherry-pick r291281. rdar://problem/90245637
Crash in KeyframeList.cpp:183 in WebCore::KeyframeList::fillImplicitKeyframes
Added: branches/safari-613-branch/LayoutTests/fast/files/blob-with-fragment-as-frame-url-expected.txt (0 => 293070)
--- branches/safari-613-branch/LayoutTests/fast/files/blob-with-fragment-as-frame-url-expected.txt (rev 0)
+++ branches/safari-613-branch/LayoutTests/fast/files/blob-with-fragment-as-frame-url-expected.txt 2022-04-20 05:38:14 UTC (rev 293070)
@@ -0,0 +1,12 @@
+Tests the lifetime of a Blob URL with a fragment.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS iframe.contentDocument.body.innerText is "FOO"
+PASS iframe.contentWindow.location.href == blobURLWithFragment is true
+PASS iframe.contentDocument.URL == blobURLWithFragment is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: branches/safari-613-branch/LayoutTests/fast/files/blob-with-fragment-as-frame-url.html (0 => 293070)
--- branches/safari-613-branch/LayoutTests/fast/files/blob-with-fragment-as-frame-url.html (rev 0)
+++ branches/safari-613-branch/LayoutTests/fast/files/blob-with-fragment-as-frame-url.html 2022-04-20 05:38:14 UTC (rev 293070)
@@ -0,0 +1,30 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<script>
+description("Tests the lifetime of a Blob URL with a fragment.");
+jsTestIsAsync = true;
+
+_onload_ = () => {
+ let blobContents = `<body>FOO</body>`;
+ let blob = new Blob([blobContents], { type: "text/html" });
+ blobURL = URL.createObjectURL(blob);
+ blobURLWithFragment = blobURL + "#foo=1";
+ iframe = document.getElementById("testFrame");
+ iframe._onload_ = () => {
+ shouldBeEqualToString("iframe.contentDocument.body.innerText", "FOO");
+ shouldBeTrue("iframe.contentWindow.location.href == blobURLWithFragment");
+ shouldBeTrue("iframe.contentDocument.URL == blobURLWithFragment");
+ finishJSTest();
+ };
+ let anchor = document.createElement("a");
+ anchor.target = "testFrame";
+ anchor.href = ""
+ anchor.click();
+ URL.revokeObjectURL(blobURL);
+}
+</script>
+<iframe id="testFrame" name="testFrame" src=""
+</body>
+</html>
Modified: branches/safari-613-branch/Source/WebCore/ChangeLog (293069 => 293070)
--- branches/safari-613-branch/Source/WebCore/ChangeLog 2022-04-20 05:38:09 UTC (rev 293069)
+++ branches/safari-613-branch/Source/WebCore/ChangeLog 2022-04-20 05:38:14 UTC (rev 293070)
@@ -1,5 +1,88 @@
2022-04-19 Alan Coon <[email protected]>
+ Cherry-pick r291689. rdar://problem/88461772
+
+ REGRESSION (iOS 15.1 / r280824) QuickLook - model not loading when passing extra parameters
+ https://bugs.webkit.org/show_bug.cgi?id=236069
+ <rdar://problem/88461772>
+
+ Reviewed by Darin Adler.
+
+ Source/WebCore:
+
+ To extend the lifetime of Blob objects while loading/downloading is pending, we rely to
+ BlobURLHandle objects, which tell the NetworkProcess that we still need this blob data,
+ even if the corresponding Blob object gets GC'd by JS. The issue here is that the load
+ was not using the vanilla blob URL but was intead appending a fragment to the URL. As
+ a result, BlobRegistryImpl::registerBlobURLHandle() would fail to look up the
+ corresponding blob data and would fail to extend its lifetime.
+
+ To address the issue, BlobRegistryImpl::registerBlobURLHandle() / unregisterBlobURLHandle()
+ now strip fragments from the blob URL, similarly to what was already done inside
+ BlobRegistryImpl::getBlobDataFromURL().
+
+ Test: fast/files/blob-with-fragment-as-frame-url.html
+
+ * platform/network/BlobRegistryImpl.cpp:
+ (WebCore::blobURLWithoutFragment):
+ (WebCore::BlobRegistryImpl::registerBlobURLHandle):
+ (WebCore::BlobRegistryImpl::unregisterBlobURLHandle):
+
+ Source/WebKit:
+
+ Fix theoretical blob data leak. A WebContent process could create several BlobURLHandles for
+ the same blob URL. This would result in several calls to NetworkConnectionToWebProcess::registerBlobURLHandle()
+ for the same URL and several calls to to BlobRegistryImpl::registerBlobURLHandle() for the
+ same URL as well. BlobRegistryImpl is using a HashCountedSet for m_blobReferences in order
+ to deal with this fact. However, NetworkConnectionToWebProcess was using a simple HashSet
+ for m_blobURLHandles. As a result, if the WebContent process would exit and didClose()
+ would get called, the NetworkConnectionToWebProcess may call BlobRegistryImpl::unregisterBlobURLHandle()
+ only once even though the WebContent process had several handles for this URL, which would
+ not fully remove the URL from BlobRegistryImpl's HashCountedSet. To address the issue,
+ NetworkConnectionToWebProcess::m_blobURLHandles is now a HashCountedSet too and we call
+ BlobRegistryImpl::unregisterBlobURLHandle() as many times as needed in didClose().
+
+ * NetworkProcess/NetworkConnectionToWebProcess.cpp:
+ (WebKit::NetworkConnectionToWebProcess::didClose):
+ * NetworkProcess/NetworkConnectionToWebProcess.h:
+
+ LayoutTests:
+
+ Add layout test coverage.
+
+ * fast/files/blob-with-fragment-as-frame-url-expected.txt: Added.
+ * fast/files/blob-with-fragment-as-frame-url.html: Added.
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@291689 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2022-03-22 Chris Dumez <[email protected]>
+
+ REGRESSION (iOS 15.1 / r280824) QuickLook - model not loading when passing extra parameters
+ https://bugs.webkit.org/show_bug.cgi?id=236069
+ <rdar://problem/88461772>
+
+ Reviewed by Darin Adler.
+
+ To extend the lifetime of Blob objects while loading/downloading is pending, we rely to
+ BlobURLHandle objects, which tell the NetworkProcess that we still need this blob data,
+ even if the corresponding Blob object gets GC'd by JS. The issue here is that the load
+ was not using the vanilla blob URL but was intead appending a fragment to the URL. As
+ a result, BlobRegistryImpl::registerBlobURLHandle() would fail to look up the
+ corresponding blob data and would fail to extend its lifetime.
+
+ To address the issue, BlobRegistryImpl::registerBlobURLHandle() / unregisterBlobURLHandle()
+ now strip fragments from the blob URL, similarly to what was already done inside
+ BlobRegistryImpl::getBlobDataFromURL().
+
+ Test: fast/files/blob-with-fragment-as-frame-url.html
+
+ * platform/network/BlobRegistryImpl.cpp:
+ (WebCore::blobURLWithoutFragment):
+ (WebCore::BlobRegistryImpl::registerBlobURLHandle):
+ (WebCore::BlobRegistryImpl::unregisterBlobURLHandle):
+
+2022-04-19 Alan Coon <[email protected]>
+
Cherry-pick r291281. rdar://problem/90245637
Crash in KeyframeList.cpp:183 in WebCore::KeyframeList::fillImplicitKeyframes
Modified: branches/safari-613-branch/Source/WebCore/platform/network/BlobRegistryImpl.cpp (293069 => 293070)
--- branches/safari-613-branch/Source/WebCore/platform/network/BlobRegistryImpl.cpp 2022-04-20 05:38:09 UTC (rev 293069)
+++ branches/safari-613-branch/Source/WebCore/platform/network/BlobRegistryImpl.cpp 2022-04-20 05:38:14 UTC (rev 293070)
@@ -50,6 +50,11 @@
namespace WebCore {
+static String blobURLWithoutFragment(const URL& url)
+{
+ return url.hasFragmentIdentifier() ? url.stringWithoutFragmentIdentifier().toString() : url.string();
+}
+
BlobRegistryImpl::~BlobRegistryImpl() = default;
static Ref<ResourceHandle> createBlobResourceHandle(const ResourceRequest& request, ResourceHandleClient* client)
@@ -377,14 +382,16 @@
void BlobRegistryImpl::registerBlobURLHandle(const URL& url)
{
- if (m_blobs.contains(url.string()))
- m_blobReferences.add(url.string());
+ auto urlKey = blobURLWithoutFragment(url);
+ if (m_blobs.contains(urlKey))
+ m_blobReferences.add(urlKey);
}
void BlobRegistryImpl::unregisterBlobURLHandle(const URL& url)
{
- if (m_blobReferences.remove(url.string()))
- m_blobs.remove(url.string());
+ auto urlKey = blobURLWithoutFragment(url);
+ if (m_blobReferences.remove(urlKey))
+ m_blobs.remove(urlKey);
}
} // namespace WebCore
Modified: branches/safari-613-branch/Source/WebKit/ChangeLog (293069 => 293070)
--- branches/safari-613-branch/Source/WebKit/ChangeLog 2022-04-20 05:38:09 UTC (rev 293069)
+++ branches/safari-613-branch/Source/WebKit/ChangeLog 2022-04-20 05:38:14 UTC (rev 293070)
@@ -1,5 +1,86 @@
2022-04-19 Alan Coon <[email protected]>
+ Cherry-pick r291689. rdar://problem/88461772
+
+ REGRESSION (iOS 15.1 / r280824) QuickLook - model not loading when passing extra parameters
+ https://bugs.webkit.org/show_bug.cgi?id=236069
+ <rdar://problem/88461772>
+
+ Reviewed by Darin Adler.
+
+ Source/WebCore:
+
+ To extend the lifetime of Blob objects while loading/downloading is pending, we rely to
+ BlobURLHandle objects, which tell the NetworkProcess that we still need this blob data,
+ even if the corresponding Blob object gets GC'd by JS. The issue here is that the load
+ was not using the vanilla blob URL but was intead appending a fragment to the URL. As
+ a result, BlobRegistryImpl::registerBlobURLHandle() would fail to look up the
+ corresponding blob data and would fail to extend its lifetime.
+
+ To address the issue, BlobRegistryImpl::registerBlobURLHandle() / unregisterBlobURLHandle()
+ now strip fragments from the blob URL, similarly to what was already done inside
+ BlobRegistryImpl::getBlobDataFromURL().
+
+ Test: fast/files/blob-with-fragment-as-frame-url.html
+
+ * platform/network/BlobRegistryImpl.cpp:
+ (WebCore::blobURLWithoutFragment):
+ (WebCore::BlobRegistryImpl::registerBlobURLHandle):
+ (WebCore::BlobRegistryImpl::unregisterBlobURLHandle):
+
+ Source/WebKit:
+
+ Fix theoretical blob data leak. A WebContent process could create several BlobURLHandles for
+ the same blob URL. This would result in several calls to NetworkConnectionToWebProcess::registerBlobURLHandle()
+ for the same URL and several calls to to BlobRegistryImpl::registerBlobURLHandle() for the
+ same URL as well. BlobRegistryImpl is using a HashCountedSet for m_blobReferences in order
+ to deal with this fact. However, NetworkConnectionToWebProcess was using a simple HashSet
+ for m_blobURLHandles. As a result, if the WebContent process would exit and didClose()
+ would get called, the NetworkConnectionToWebProcess may call BlobRegistryImpl::unregisterBlobURLHandle()
+ only once even though the WebContent process had several handles for this URL, which would
+ not fully remove the URL from BlobRegistryImpl's HashCountedSet. To address the issue,
+ NetworkConnectionToWebProcess::m_blobURLHandles is now a HashCountedSet too and we call
+ BlobRegistryImpl::unregisterBlobURLHandle() as many times as needed in didClose().
+
+ * NetworkProcess/NetworkConnectionToWebProcess.cpp:
+ (WebKit::NetworkConnectionToWebProcess::didClose):
+ * NetworkProcess/NetworkConnectionToWebProcess.h:
+
+ LayoutTests:
+
+ Add layout test coverage.
+
+ * fast/files/blob-with-fragment-as-frame-url-expected.txt: Added.
+ * fast/files/blob-with-fragment-as-frame-url.html: Added.
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@291689 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2022-03-22 Chris Dumez <[email protected]>
+
+ REGRESSION (iOS 15.1 / r280824) QuickLook - model not loading when passing extra parameters
+ https://bugs.webkit.org/show_bug.cgi?id=236069
+ <rdar://problem/88461772>
+
+ Reviewed by Darin Adler.
+
+ Fix theoretical blob data leak. A WebContent process could create several BlobURLHandles for
+ the same blob URL. This would result in several calls to NetworkConnectionToWebProcess::registerBlobURLHandle()
+ for the same URL and several calls to to BlobRegistryImpl::registerBlobURLHandle() for the
+ same URL as well. BlobRegistryImpl is using a HashCountedSet for m_blobReferences in order
+ to deal with this fact. However, NetworkConnectionToWebProcess was using a simple HashSet
+ for m_blobURLHandles. As a result, if the WebContent process would exit and didClose()
+ would get called, the NetworkConnectionToWebProcess may call BlobRegistryImpl::unregisterBlobURLHandle()
+ only once even though the WebContent process had several handles for this URL, which would
+ not fully remove the URL from BlobRegistryImpl's HashCountedSet. To address the issue,
+ NetworkConnectionToWebProcess::m_blobURLHandles is now a HashCountedSet too and we call
+ BlobRegistryImpl::unregisterBlobURLHandle() as many times as needed in didClose().
+
+ * NetworkProcess/NetworkConnectionToWebProcess.cpp:
+ (WebKit::NetworkConnectionToWebProcess::didClose):
+ * NetworkProcess/NetworkConnectionToWebProcess.h:
+
+2022-04-19 Alan Coon <[email protected]>
+
Cherry-pick r291371. rdar://problem/72058321
CoreIPC Hardening: Add user gesture check when saving images
Modified: branches/safari-613-branch/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp (293069 => 293070)
--- branches/safari-613-branch/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp 2022-04-20 05:38:09 UTC (rev 293069)
+++ branches/safari-613-branch/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp 2022-04-20 05:38:14 UTC (rev 293070)
@@ -388,8 +388,10 @@
networkSession->broadcastChannelRegistry().removeConnection(connection);
for (auto& url : m_blobURLs)
networkSession->blobRegistry().unregisterBlobURL(url);
- for (auto& url : m_blobURLHandles)
- networkSession->blobRegistry().unregisterBlobURLHandle(url);
+ for (auto& [url, count] : m_blobURLHandles) {
+ for (unsigned i = 0; i < count; ++i)
+ networkSession->blobRegistry().unregisterBlobURLHandle(url);
+ }
}
// All trackers of resources that were in the middle of being loaded were
Modified: branches/safari-613-branch/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h (293069 => 293070)
--- branches/safari-613-branch/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h 2022-04-20 05:38:09 UTC (rev 293069)
+++ branches/safari-613-branch/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h 2022-04-20 05:38:14 UTC (rev 293070)
@@ -416,7 +416,7 @@
Ref<NetworkSchemeRegistry> m_schemeRegistry;
HashSet<URL> m_blobURLs;
- HashSet<URL> m_blobURLHandles;
+ HashCountedSet<URL> m_blobURLHandles;
};
} // namespace WebKit
