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

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

 /*
  * Copyright (C) 2006 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) Research In Motion Limited 2011. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
@@ skipping 49 matching lines @@
 #include "platform/graphics/paint/PaintRecord.h"
 #include "platform/graphics/paint/PaintRecordBuilder.h"
 #include "platform/instrumentation/tracing/TraceEvent.h"
 #include "wtf/PassRefPtr.h"
 
 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) {
     // Store m_page in a local variable, clearing m_page, so that
     // SVGImageChromeClient knows we're destructed.
     Page* currentPage = m_page.release();
     // Break both the loader and view references to the frame
     currentPage->willBeDestroyed();
   }
 
@@ skipping 472 matching lines @@
 }
 
 void SVGImage::updateUseCounters(const Document& document) const {
   if (SVGSVGElement* rootElement = svgRootElement(m_page.get())) {
     if (rootElement->timeContainer()->hasAnimations())
       UseCounter::count(document,
                         UseCounter::SVGSMILAnimationInImageRegardlessOfCache);
   }
 }
 
+// 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");
 
   // Don't do anything if is an empty image.
   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);
     m_chromeClient = SVGImageChromeClient::create(this);
     pageClients.chromeClient = m_chromeClient.get();
 
     // FIXME: If this SVG ends up loading itself, we might leak the world.
     // The Cache code does not know about ImageResources holding Frames and
     // won't know to break the cycle.
     // This will become an issue when SVGImage will be able to load other
@@ skipping 22 matching lines @@
         page->settings().setDefaultFontSize(
             defaultSettings.getDefaultFontSize());
         page->settings().setDefaultFixedFontSize(
             defaultSettings.getDefaultFixedFontSize());
       }
     }
 
     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();
     }
 
     FrameLoader& loader = frame->loader();
     loader.forceSandboxFlags(SandboxAll);
 
     frame->view()->setScrollbarsSuppressed(true);
     // SVG Images will always synthesize a viewBox, if it's not available, and
     // thus never see scrollbars.
     frame->view()->setCanHaveScrollbars(false);
     // SVG Images are transparent.
     frame->view()->setTransparent(true);
 
     m_page = page;
 
     TRACE_EVENT0("blink", "SVGImage::dataChanged::load");
     loader.load(FrameLoadRequest(
         0, blankURL(),
         SubstituteData(data(), AtomicString("image/svg+xml"),
                        AtomicString("UTF-8"), KURL(), ForceSynchronousLoad)));
 
     // Set the concrete object size before a container size is available.
     m_intrinsicSize = roundedIntSize(concreteObjectSize(FloatSize(
         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 {
   return "svg";
 }
 
 }  // namespace blink