Release assert in ScriptController::canExecuteScripts via WebCore::SVGUseElement...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Jun 2018 21:00:45 +0000 (21:00 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Jun 2018 21:00:45 +0000 (21:00 +0000)
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:

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

Source/WebCore/ChangeLog
Source/WebCore/loader/cache/CachedResource.cpp
Source/WebCore/svg/SVGUseElement.cpp
Source/WebCore/svg/SVGUseElement.h

index 1123bbe..d3e13aa 100644 (file)
@@ -1,3 +1,29 @@
+2018-06-28  Ryosuke Niwa  <rniwa@webkit.org>
+
+        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  <cdumez@apple.com>
 
         Unreviewed, rolling out r233309.
 2018-06-28  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed, rolling out r233309.
index 8be377a..88cc634 100644 (file)
@@ -505,6 +505,8 @@ void CachedResource::didAddClient(CachedResourceClient& client)
 
     if (m_clientsAwaitingCallback.remove(&client))
         m_clients.add(&client);
 
     if (m_clientsAwaitingCallback.remove(&client))
         m_clients.add(&client);
+
+    // FIXME: Make calls to notifyFinished async
     if (!isLoading() && !stillNeedsLoad())
         client.notifyFinished(*this);
 }
     if (!isLoading() && !stillNeedsLoad())
         client.notifyFinished(*this);
 }
index 9b5ffb1..525292d 100644 (file)
@@ -115,11 +115,17 @@ Node::InsertedIntoAncestorResult SVGUseElement::insertedIntoAncestor(InsertionTy
             document().addSVGUseElement(*this);
         SVGExternalResourcesRequired::insertedIntoDocument(this);
         invalidateShadowTree();
             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;
 }
 
     }
     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
 void SVGUseElement::removedFromAncestor(RemovalType removalType, ContainerNode& oldParentOfRemovedTree)
 {
     // Check m_shadowTreeNeedsUpdate before calling SVGElement::removedFromAncestor which calls SVGElement::invalidateInstances
@@ -558,6 +564,7 @@ bool SVGUseElement::selfHasRelativeLengths() const
 
 void SVGUseElement::notifyFinished(CachedResource& resource)
 {
 
 void SVGUseElement::notifyFinished(CachedResource& resource)
 {
+    ASSERT(ScriptDisallowedScope::InMainThread::isScriptAllowed());
     invalidateShadowTree();
     if (resource.errorOccurred())
         dispatchEvent(Event::create(eventNames().errorEvent, false, false));
     invalidateShadowTree();
     if (resource.errorOccurred())
         dispatchEvent(Event::create(eventNames().errorEvent, false, false));
index 069ad28..65060bb 100644 (file)
@@ -61,6 +61,7 @@ private:
 
     bool isValid() const override;
     InsertedIntoAncestorResult insertedIntoAncestor(InsertionType, ContainerNode&) override;
 
     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;
     void removedFromAncestor(RemovalType, ContainerNode&) override;
     void buildPendingResource() override;
     void parseAttribute(const QualifiedName&, const AtomicString&) override;