Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes

third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp

Index: third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
diff --git a/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp b/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
index 46628934b330656e7879b91b4065efa2db80b819..551459fd0b78451c3c8604c86589ce4e987f419c 100644
--- a/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
+++ b/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
@@ -67,7 +67,8 @@ namespace blink {
 SVGImage::SVGImage(ImageObserver* observer)
     : Image(observer),
       m_paintController(PaintController::create()),
-      m_hasPendingTimelineRewind(false) {}
+      m_hasPendingTimelineRewind(false),
+      m_weakPtrFactory(this) {}
 
 SVGImage::~SVGImage() {
   if (m_page) {
@@ -560,6 +561,39 @@ void SVGImage::updateUseCounters(const Document& document) const {
   }
 }
 
+// SVGImageLocalFrameClient is used to wait until SVG document's load event
+// in the case where there are subresources asynchronously loaded.
+class SVGImage::SVGImageLocalFrameClient : public EmptyLocalFrameClient {
+ public:
+  SVGImageLocalFrameClient(WeakPtr<SVGImage> image) : m_image(image) {}
+
+ private:
+  void dispatchDidHandleOnloadEvents() override {
+    // The SVGImage is destructed before SVG load completion.
+    if (!m_image)
+      return;
+
+    switch (m_image->m_loadStatus) {

[fs, 2017/03/15 14:22:17]

Could we have this just call a method on SVGImage, and thus keep all the state
management there?

[hiroshige, 2017/05/04 22:50:50]

Done.

+      case InDataChanged:
+        m_image->m_loadStatus = LoadCompletedWithinDataChanged;
+        break;
+
+      case AfterDataChanged:
+        m_image->m_loadStatus = LoadCompletedAsynchronously;
+        if (m_image->getImageObserver())
+          m_image->getImageObserver()->loadCompleted(m_image.get());
+        break;
+
+      case DataChangedNotStarted:
+      case LoadCompletedWithinDataChanged:
+      case LoadCompletedAsynchronously:
+        CHECK(false);
+        break;
+    }
+  }
+  WeakPtr<SVGImage> m_image;
+};
+
 Image::SizeAvailability SVGImage::dataChanged(bool allDataReceived) {
   TRACE_EVENT0("blink", "SVGImage::dataChanged");
 
@@ -567,17 +601,20 @@ Image::SizeAvailability SVGImage::dataChanged(bool allDataReceived) {
   if (!data()->size())
     return SizeAvailable;
 
-  if (allDataReceived) {
+  if (!allDataReceived)
+    return m_page ? SizeAvailable : SizeUnavailable;
+
+  CHECK(!m_page);
+
+  {

[kouhei (in TOK), 2017/03/15 11:32:34]

remove { and unindent

[hiroshige, 2017/05/04 22:50:50]

Done.

     // SVGImage will fire events (and the default C++ handlers run) but doesn't
     // actually allow script to run so it's fine to call into it. We allow this
     // since it means an SVG data url can synchronously load like other image
     // types.
     EventDispatchForbiddenScope::AllowUserAgentEvents allowUserAgentEvents;
 
-    DEFINE_STATIC_LOCAL(LocalFrameClient, dummyLocalFrameClient,
-                        (EmptyLocalFrameClient::create()));
-
-    CHECK(!m_page);
+    CHECK_EQ(m_loadStatus, DataChangedNotStarted);
+    m_loadStatus = InDataChanged;
 
     Page::PageClients pageClients;
     fillWithEmptyClients(pageClients);
@@ -620,7 +657,9 @@ Image::SizeAvailability SVGImage::dataChanged(bool allDataReceived) {
     LocalFrame* frame = nullptr;
     {
       TRACE_EVENT0("blink", "SVGImage::dataChanged::createFrame");
-      frame = LocalFrame::create(&dummyLocalFrameClient, &page->frameHost(), 0);
+      frame =
+          LocalFrame::create(new SVGImageLocalFrameClient(this->asWeakPtr()),

[fs, 2017/03/15 14:22:17]

What is the case that prompts the use of a WeakPtr here? (The FrameClient is on
the GC heap.)

[hiroshige, 2017/05/04 22:50:50]

While I don't know concrete cases, the following is what I thought:
Previously the FrameClient doesn't have strong references to SVGImage, so I made
this WeakPtr to keep the lifetime implication.
Also SVGImage is ThreadSafeRefCounted and thus having RefPtr here might cause
changes in the SVGImage destruction timing.

[fs, 2017/05/05 10:52:42]

Yes, a RefPtr would certainly not work. With the current setup we'll tear down
the Page (SVGImage::page_) before any weak pointers are invalidated though, so
we should probably at least explicitly revoke the WeakPtr before tearing down
the Page, or this setup will not be meaningful AFAICS.

[hiroshige, 2017/05/08 17:22:06]

Oh, and I noticed we cannot use WeakPtr with ThreadSafeRefCounted.

[hiroshige, 2017/05/08 18:37:11]

So, this means SVGImage is always alive when the notification is called, right?
If this is the only case where WeakPtr should be null at the notification, we
can check SVGImage::page_ instead (because page_ is already nullptr when thepage
is being tearing down) and use a raw pointer instead of WeakPtr.

However, raw pointers looks unsafe and I prefer WeakPtr though.

Filed crbug.com/719617 for Image being ThreadSafeRefCounted.

[fs, 2017/05/08 18:52:33]

Yes, AFAICS.
Thanks.

+                             &page->frameHost(), 0);
       frame->setView(FrameView::create(*frame));
       frame->init();
     }
@@ -648,7 +687,24 @@ Image::SizeAvailability SVGImage::dataChanged(bool allDataReceived) {
         LayoutReplaced::defaultWidth, LayoutReplaced::defaultHeight)));
   }
 
-  return m_page ? SizeAvailable : SizeUnavailable;
+  DCHECK(m_page);
+  switch (m_loadStatus) {
+    case InDataChanged:
+      m_loadStatus = AfterDataChanged;
+      return SizeAvailableAndLoadingAsynchronously;
+
+    case LoadCompletedWithinDataChanged:
+      return SizeAvailable;
+
+    case DataChangedNotStarted:
+    case AfterDataChanged:
+    case LoadCompletedAsynchronously:
+      CHECK(false);
+      break;
+  }
+
+  NOTREACHED();
+  return SizeAvailable;
 }
 
 String SVGImage::filenameExtension() const {