Title: [233324] trunk/Source/WebCore
- Revision
- 233324
- Author
- [email protected]
- Date
- 2018-06-28 14:00:45 -0700 (Thu, 28 Jun 2018)
Log Message
Release assert in ScriptController::canExecuteScripts via WebCore::SVGUseElement::insertedIntoAncestor
https://bugs.webkit.org/show_bug.cgi?id=187137
<rdar://problem/41081885>
Reviewed by Zalan Bujtas.
The bug was caused by SVGUseElement::notifyFinished firing a DOM event via SVGUseElement::updateExternalDocument
inside SVGUseElement::insertedIntoAncestor. Ideally, we make every call to notifyFinished asynchronous
but simply delay the call to updateExternalDocument() until didFinishInsertingNode() for now.
No new tests since the failure is caught with the newly added assertion in notifyFinished by existing SVG tests
such as svg/batik/filters/filterRegions.svg and svg/batik/text/smallFonts.svg. Unfortunately, I could not
construct a test case which hits this release assertion since the real crash happens when the cached resource
had an error but in the all cases I could find, the resource response with an error results in a reload or
an asynchronous failure callback.
* loader/cache/CachedResource.cpp:
(WebCore::CachedResource::didAddClient): Added a FIXME.
* svg/SVGUseElement.cpp:
(WebCore::SVGUseElement::insertedIntoAncestor): Delay the call to updateExternalDocument.
(WebCore::SVGUseElement::didFinishInsertingNode): Invoke updateExternalDocument.
(WebCore::SVGUseElement::notifyFinished): Added an assertion.
* svg/SVGUseElement.h:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (233323 => 233324)
--- trunk/Source/WebCore/ChangeLog 2018-06-28 20:59:19 UTC (rev 233323)
+++ trunk/Source/WebCore/ChangeLog 2018-06-28 21:00:45 UTC (rev 233324)
@@ -1,3 +1,29 @@
+2018-06-28 Ryosuke Niwa <[email protected]>
+
+ Release assert in ScriptController::canExecuteScripts via WebCore::SVGUseElement::insertedIntoAncestor
+ https://bugs.webkit.org/show_bug.cgi?id=187137
+ <rdar://problem/41081885>
+
+ Reviewed by Zalan Bujtas.
+
+ The bug was caused by SVGUseElement::notifyFinished firing a DOM event via SVGUseElement::updateExternalDocument
+ inside SVGUseElement::insertedIntoAncestor. Ideally, we make every call to notifyFinished asynchronous
+ but simply delay the call to updateExternalDocument() until didFinishInsertingNode() for now.
+
+ No new tests since the failure is caught with the newly added assertion in notifyFinished by existing SVG tests
+ such as svg/batik/filters/filterRegions.svg and svg/batik/text/smallFonts.svg. Unfortunately, I could not
+ construct a test case which hits this release assertion since the real crash happens when the cached resource
+ had an error but in the all cases I could find, the resource response with an error results in a reload or
+ an asynchronous failure callback.
+
+ * loader/cache/CachedResource.cpp:
+ (WebCore::CachedResource::didAddClient): Added a FIXME.
+ * svg/SVGUseElement.cpp:
+ (WebCore::SVGUseElement::insertedIntoAncestor): Delay the call to updateExternalDocument.
+ (WebCore::SVGUseElement::didFinishInsertingNode): Invoke updateExternalDocument.
+ (WebCore::SVGUseElement::notifyFinished): Added an assertion.
+ * svg/SVGUseElement.h:
+
2018-06-28 Chris Dumez <[email protected]>
Unreviewed, rolling out r233309.
Modified: trunk/Source/WebCore/loader/cache/CachedResource.cpp (233323 => 233324)
--- trunk/Source/WebCore/loader/cache/CachedResource.cpp 2018-06-28 20:59:19 UTC (rev 233323)
+++ trunk/Source/WebCore/loader/cache/CachedResource.cpp 2018-06-28 21:00:45 UTC (rev 233324)
@@ -505,6 +505,8 @@
if (m_clientsAwaitingCallback.remove(&client))
m_clients.add(&client);
+
+ // FIXME: Make calls to notifyFinished async
if (!isLoading() && !stillNeedsLoad())
client.notifyFinished(*this);
}
Modified: trunk/Source/WebCore/svg/SVGUseElement.cpp (233323 => 233324)
--- trunk/Source/WebCore/svg/SVGUseElement.cpp 2018-06-28 20:59:19 UTC (rev 233323)
+++ trunk/Source/WebCore/svg/SVGUseElement.cpp 2018-06-28 21:00:45 UTC (rev 233324)
@@ -115,11 +115,17 @@
document().addSVGUseElement(*this);
SVGExternalResourcesRequired::insertedIntoDocument(this);
invalidateShadowTree();
- updateExternalDocument();
+ // FIXME: Move back the call to updateExternalDocument() here once notifyFinished is made always async.
+ return InsertedIntoAncestorResult::NeedsPostInsertionCallback;
}
return InsertedIntoAncestorResult::Done;
}
+void SVGUseElement::didFinishInsertingNode()
+{
+ updateExternalDocument();
+}
+
void SVGUseElement::removedFromAncestor(RemovalType removalType, ContainerNode& oldParentOfRemovedTree)
{
// Check m_shadowTreeNeedsUpdate before calling SVGElement::removedFromAncestor which calls SVGElement::invalidateInstances
@@ -558,6 +564,7 @@
void SVGUseElement::notifyFinished(CachedResource& resource)
{
+ ASSERT(ScriptDisallowedScope::InMainThread::isScriptAllowed());
invalidateShadowTree();
if (resource.errorOccurred())
dispatchEvent(Event::create(eventNames().errorEvent, false, false));
Modified: trunk/Source/WebCore/svg/SVGUseElement.h (233323 => 233324)
--- trunk/Source/WebCore/svg/SVGUseElement.h 2018-06-28 20:59:19 UTC (rev 233323)
+++ trunk/Source/WebCore/svg/SVGUseElement.h 2018-06-28 21:00:45 UTC (rev 233324)
@@ -61,6 +61,7 @@
bool isValid() const override;
InsertedIntoAncestorResult insertedIntoAncestor(InsertionType, ContainerNode&) override;
+ void didFinishInsertingNode() final;
void removedFromAncestor(RemovalType, ContainerNode&) override;
void buildPendingResource() override;
void parseAttribute(const QualifiedName&, const AtomicString&) override;
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
