- Revision
- 162643
- Author
- jhoneyc...@apple.com
- Date
- 2014-01-23 14:24:03 -0800 (Thu, 23 Jan 2014)
Log Message
REGRESSION(r161967): Crash in WebCore::CachedSVGDocumentReference::load
<https://webkit.org/b/127151>
<rdar://problem/15840760>
Source/WebCore:
There were two issues introduced here; the first is a use-after-free of
CachedSVGDocumentReference objects.
The previous code kept a map from FilterOperation ->
RefPtr<WebKitCSSSVGDocumentValue>, which retained the
CachedSVGDocument. In r161967, this was changed to use a weak HashSet,
which allows stale CachedSVGDocumentReferences in the pending document
set if the owning FilterOperation is deleted. To fix this, we'll keep a
vector of RefPtr<FilterOperation> with pending SVG documents.
The second issue is a null deref in CachedSVGDocumentReference::load();
CachedResourceLoader::requestSVGDocument() can return 0 if (for
example) an invalid URL is passed. r161967 removed a null check as part
of the refactoring.
Reviewed by Dirk Schulze.
Tests: css3/filters/crash-filter-animation-invalid-url.html
css3/filters/crash-invalid-url.html
* css/StyleResolver.cpp:
(WebCore::StyleResolver::State::clear):
Use new member var name.
(WebCore::StyleResolver::loadPendingSVGDocuments):
For each FilterOperation with a pending SVG document, get or create a
CachedSVGDocumentReference, and tell it to load. Changed to use new
function names.
(WebCore::StyleResolver::createFilterOperations):
Append the FilterOperation to the list of FilterOperations with
unloaded SVG documents.
* css/StyleResolver.h:
Changed from using PendingSVGDocumentSet, a weak set, to
a Vector<RefPtr<ReferenceFilterOperation>>.
(WebCore::StyleResolver::State::filtersWithPendingSVGDocuments):
Return the vector.
* loader/cache/CachedSVGDocumentReference.cpp:
(WebCore::CachedSVGDocumentReference::~CachedSVGDocumentReference):
Null check m_document rather than checking m_loadRequested.
m_loadRequested may be true when m_document is 0.
(WebCore::CachedSVGDocumentReference::load):
Null check the result of CachedResourceLoader::requestSVGDocument().
* platform/graphics/filters/FilterOperation.cpp:
(WebCore::ReferenceFilterOperation::getOrCreateCachedSVGDocumentReference):
Create, if necessary, and return the CachedSVGDocumentReference.
* platform/graphics/filters/FilterOperation.h:
Replaced createCachedSVGDocumentReference() with
getOrCreateCachedSVGDocumentReference(), which makes for slightly
cleaner code.
LayoutTests:
Reviewed by Dirk Schulze.
* css3/filters/crash-filter-animation-invalid-url-expected.txt: Added.
* css3/filters/crash-filter-animation-invalid-url.html: Added.
* css3/filters/crash-invalid-url-expected.txt: Added.
* css3/filters/crash-invalid-url.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (162642 => 162643)
--- trunk/LayoutTests/ChangeLog 2014-01-23 22:12:08 UTC (rev 162642)
+++ trunk/LayoutTests/ChangeLog 2014-01-23 22:24:03 UTC (rev 162643)
@@ -1,3 +1,16 @@
+2014-01-22 Jon Honeycutt <jhoneyc...@apple.com>
+
+ REGRESSION(r161967): Crash in WebCore::CachedSVGDocumentReference::load
+ <https://webkit.org/b/127151>
+ <rdar://problem/15840760>
+
+ Reviewed by Dirk Schulze.
+
+ * css3/filters/crash-filter-animation-invalid-url-expected.txt: Added.
+ * css3/filters/crash-filter-animation-invalid-url.html: Added.
+ * css3/filters/crash-invalid-url-expected.txt: Added.
+ * css3/filters/crash-invalid-url.html: Added.
+
2014-01-23 Alexey Proskuryakov <a...@apple.com>
svg/animations/smil-syncbase-self-dependency.svg is very flaky
Added: trunk/LayoutTests/css3/filters/crash-filter-animation-invalid-url-expected.txt (0 => 162643)
--- trunk/LayoutTests/css3/filters/crash-filter-animation-invalid-url-expected.txt (rev 0)
+++ trunk/LayoutTests/css3/filters/crash-filter-animation-invalid-url-expected.txt 2014-01-23 22:24:03 UTC (rev 162643)
@@ -0,0 +1,3 @@
+WebKit bug #127151. This test passes if it doesn't crash.
+
+XXX
Added: trunk/LayoutTests/css3/filters/crash-filter-animation-invalid-url.html (0 => 162643)
--- trunk/LayoutTests/css3/filters/crash-filter-animation-invalid-url.html (rev 0)
+++ trunk/LayoutTests/css3/filters/crash-filter-animation-invalid-url.html 2014-01-23 22:24:03 UTC (rev 162643)
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<!-- Test passes if doesn't crash. -->
+<style>
+ a {
+ -webkit-animation-name: n;
+ -webkit-animation-duration: .01s;
+ }
+ @-webkit-keyframes n {
+ from {
+ -webkit-filter: url(z);
+ }
+ }
+</style>
+
+
+WebKit bug #127151. This test passes if it doesn't crash.
+
+<br><br>
+<a href="" id="a">XXX</div>
+
+<script>
+ if (window.testRunner) {
+ window.testRunner.dumpAsText(true);
+ window.testRunner.waitUntilDone();
+ }
+ document.getElementById("a").addEventListener('webkitAnimationStart', function() {
+ window.testRunner.notifyDone();
+ }, false);
+</script>
Added: trunk/LayoutTests/css3/filters/crash-invalid-url-expected.txt (0 => 162643)
--- trunk/LayoutTests/css3/filters/crash-invalid-url-expected.txt (rev 0)
+++ trunk/LayoutTests/css3/filters/crash-invalid-url-expected.txt 2014-01-23 22:24:03 UTC (rev 162643)
@@ -0,0 +1 @@
+WebKit bug #127151. This test passes if it doesn't crash.
Added: trunk/LayoutTests/css3/filters/crash-invalid-url.html (0 => 162643)
--- trunk/LayoutTests/css3/filters/crash-invalid-url.html (rev 0)
+++ trunk/LayoutTests/css3/filters/crash-invalid-url.html 2014-01-23 22:24:03 UTC (rev 162643)
@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<!-- Test passes if doesn't crash. -->
+
+<a href="" style="-webkit-filter: url(x);">WebKit bug #127151. This test passes if it doesn't crash.</a>
+
+<script>
+ if (window.testRunner)
+ window.testRunner.dumpAsText(true);
+</script>
\ No newline at end of file
Modified: trunk/Source/WebCore/ChangeLog (162642 => 162643)
--- trunk/Source/WebCore/ChangeLog 2014-01-23 22:12:08 UTC (rev 162642)
+++ trunk/Source/WebCore/ChangeLog 2014-01-23 22:24:03 UTC (rev 162643)
@@ -1,3 +1,62 @@
+2014-01-22 Jon Honeycutt <jhoneyc...@apple.com>
+
+ REGRESSION(r161967): Crash in WebCore::CachedSVGDocumentReference::load
+ <https://webkit.org/b/127151>
+ <rdar://problem/15840760>
+
+ There were two issues introduced here; the first is a use-after-free of
+ CachedSVGDocumentReference objects.
+
+ The previous code kept a map from FilterOperation ->
+ RefPtr<WebKitCSSSVGDocumentValue>, which retained the
+ CachedSVGDocument. In r161967, this was changed to use a weak HashSet,
+ which allows stale CachedSVGDocumentReferences in the pending document
+ set if the owning FilterOperation is deleted. To fix this, we'll keep a
+ vector of RefPtr<FilterOperation> with pending SVG documents.
+
+ The second issue is a null deref in CachedSVGDocumentReference::load();
+ CachedResourceLoader::requestSVGDocument() can return 0 if (for
+ example) an invalid URL is passed. r161967 removed a null check as part
+ of the refactoring.
+
+ Reviewed by Dirk Schulze.
+
+ Tests: css3/filters/crash-filter-animation-invalid-url.html
+ css3/filters/crash-invalid-url.html
+
+ * css/StyleResolver.cpp:
+ (WebCore::StyleResolver::State::clear):
+ Use new member var name.
+ (WebCore::StyleResolver::loadPendingSVGDocuments):
+ For each FilterOperation with a pending SVG document, get or create a
+ CachedSVGDocumentReference, and tell it to load. Changed to use new
+ function names.
+ (WebCore::StyleResolver::createFilterOperations):
+ Append the FilterOperation to the list of FilterOperations with
+ unloaded SVG documents.
+
+ * css/StyleResolver.h:
+ Changed from using PendingSVGDocumentSet, a weak set, to
+ a Vector<RefPtr<ReferenceFilterOperation>>.
+ (WebCore::StyleResolver::State::filtersWithPendingSVGDocuments):
+ Return the vector.
+
+ * loader/cache/CachedSVGDocumentReference.cpp:
+ (WebCore::CachedSVGDocumentReference::~CachedSVGDocumentReference):
+ Null check m_document rather than checking m_loadRequested.
+ m_loadRequested may be true when m_document is 0.
+ (WebCore::CachedSVGDocumentReference::load):
+ Null check the result of CachedResourceLoader::requestSVGDocument().
+
+ * platform/graphics/filters/FilterOperation.cpp:
+ (WebCore::ReferenceFilterOperation::getOrCreateCachedSVGDocumentReference):
+ Create, if necessary, and return the CachedSVGDocumentReference.
+
+ * platform/graphics/filters/FilterOperation.h:
+ Replaced createCachedSVGDocumentReference() with
+ getOrCreateCachedSVGDocumentReference(), which makes for slightly
+ cleaner code.
+
2014-01-23 Antti Koivisto <an...@apple.com>
Don't enable speculative tiles immediately after main load stops progressing
Modified: trunk/Source/WebCore/css/StyleResolver.cpp (162642 => 162643)
--- trunk/Source/WebCore/css/StyleResolver.cpp 2014-01-23 22:12:08 UTC (rev 162642)
+++ trunk/Source/WebCore/css/StyleResolver.cpp 2014-01-23 22:24:03 UTC (rev 162643)
@@ -260,7 +260,7 @@
m_hasPendingShaders = false;
#endif
#if ENABLE(CSS_FILTERS) && ENABLE(SVG)
- m_pendingSVGDocuments.clear();
+ m_filtersWithPendingSVGDocuments.clear();
#endif
}
@@ -3415,13 +3415,14 @@
// style is NULL. We don't know exactly why this happens. Our guess is
// reentering styleForElement().
ASSERT(state.style());
- if (!state.style() || !state.style()->hasFilter() || state.pendingSVGDocuments().isEmpty())
+ if (!state.style() || !state.style()->hasFilter() || state.filtersWithPendingSVGDocuments().isEmpty())
return;
CachedResourceLoader* cachedResourceLoader = state.document().cachedResourceLoader();
- for (auto pendingDocument : state.pendingSVGDocuments())
- pendingDocument->load(cachedResourceLoader);
- state.pendingSVGDocuments().clear();
+ for (auto filterOperation : state.filtersWithPendingSVGDocuments())
+ filterOperation->getOrCreateCachedSVGDocumentReference()->load(cachedResourceLoader);
+
+ state.filtersWithPendingSVGDocuments().clear();
}
#endif
@@ -3818,7 +3819,8 @@
RefPtr<ReferenceFilterOperation> operation = ReferenceFilterOperation::create(cssUrl, url.fragmentIdentifier(), operationType);
if (SVGURIReference::isExternalURIReference(cssUrl, m_state.document()))
- m_state.pendingSVGDocuments().add(operation->createCachedSVGDocumentReference());
+ state.filtersWithPendingSVGDocuments().append(operation);
+
operations.operations().append(operation);
#endif
continue;
Modified: trunk/Source/WebCore/css/StyleResolver.h (162642 => 162643)
--- trunk/Source/WebCore/css/StyleResolver.h 2014-01-23 22:12:08 UTC (rev 162642)
+++ trunk/Source/WebCore/css/StyleResolver.h 2014-01-23 22:24:03 UTC (rev 162643)
@@ -374,9 +374,6 @@
public:
typedef HashMap<CSSPropertyID, RefPtr<CSSValue>> PendingImagePropertyMap;
-#if ENABLE(CSS_FILTERS) && ENABLE(SVG)
- typedef HashSet<CachedSVGDocumentReference*> PendingSVGDocumentSet;
-#endif
class State {
WTF_MAKE_NONCOPYABLE(State);
@@ -428,7 +425,7 @@
bool applyPropertyToVisitedLinkStyle() const { return m_applyPropertyToVisitedLinkStyle; }
PendingImagePropertyMap& pendingImageProperties() { return m_pendingImageProperties; }
#if ENABLE(CSS_FILTERS) && ENABLE(SVG)
- PendingSVGDocumentSet& pendingSVGDocuments() { return m_pendingSVGDocuments; }
+ Vector<RefPtr<ReferenceFilterOperation>>& filtersWithPendingSVGDocuments() { return m_filtersWithPendingSVGDocuments; }
#endif
#if ENABLE(CSS_SHADERS)
void setHasPendingShaders(bool hasPendingShaders) { m_hasPendingShaders = hasPendingShaders; }
@@ -482,7 +479,7 @@
bool m_hasPendingShaders;
#endif
#if ENABLE(CSS_FILTERS) && ENABLE(SVG)
- PendingSVGDocumentSet m_pendingSVGDocuments;
+ Vector<RefPtr<ReferenceFilterOperation>> m_filtersWithPendingSVGDocuments;
#endif
CSSValue* m_lineHeightValue;
bool m_fontDirty;
Modified: trunk/Source/WebCore/loader/cache/CachedSVGDocumentReference.cpp (162642 => 162643)
--- trunk/Source/WebCore/loader/cache/CachedSVGDocumentReference.cpp 2014-01-23 22:12:08 UTC (rev 162642)
+++ trunk/Source/WebCore/loader/cache/CachedSVGDocumentReference.cpp 2014-01-23 22:24:03 UTC (rev 162643)
@@ -44,7 +44,7 @@
CachedSVGDocumentReference::~CachedSVGDocumentReference()
{
- if (m_loadRequested)
+ if (m_document)
m_document->removeClient(this);
}
@@ -57,7 +57,9 @@
CachedResourceRequest request(ResourceRequest(loader->document()->completeURL(m_url)));
request.setInitiator(cachedResourceRequestInitiators().css);
m_document = loader->requestSVGDocument(request);
- m_document->addClient(this);
+ if (m_document)
+ m_document->addClient(this);
+
m_loadRequested = true;
}
Modified: trunk/Source/WebCore/platform/graphics/filters/FilterOperation.cpp (162642 => 162643)
--- trunk/Source/WebCore/platform/graphics/filters/FilterOperation.cpp 2014-01-23 22:12:08 UTC (rev 162642)
+++ trunk/Source/WebCore/platform/graphics/filters/FilterOperation.cpp 2014-01-23 22:24:03 UTC (rev 162643)
@@ -48,9 +48,10 @@
}
#if ENABLE(SVG)
-CachedSVGDocumentReference* ReferenceFilterOperation::createCachedSVGDocumentReference()
+CachedSVGDocumentReference* ReferenceFilterOperation::getOrCreateCachedSVGDocumentReference()
{
- m_cachedSVGDocumentReference = std::make_unique<CachedSVGDocumentReference>(m_url);
+ if (!m_cachedSVGDocumentReference)
+ m_cachedSVGDocumentReference = std::make_unique<CachedSVGDocumentReference>(m_url);
return m_cachedSVGDocumentReference.get();
}
#endif
Modified: trunk/Source/WebCore/platform/graphics/filters/FilterOperation.h (162642 => 162643)
--- trunk/Source/WebCore/platform/graphics/filters/FilterOperation.h 2014-01-23 22:12:08 UTC (rev 162642)
+++ trunk/Source/WebCore/platform/graphics/filters/FilterOperation.h 2014-01-23 22:24:03 UTC (rev 162643)
@@ -166,7 +166,7 @@
#if ENABLE(SVG)
CachedSVGDocumentReference* cachedSVGDocumentReference() const { return m_cachedSVGDocumentReference.get(); }
- CachedSVGDocumentReference* createCachedSVGDocumentReference();
+ CachedSVGDocumentReference* getOrCreateCachedSVGDocumentReference();
#endif
FilterEffect* filterEffect() const { return m_filterEffect.get(); }
