Wait to notify completion until after a Lo-Fi image is reloaded.

third_party/WebKit/Source/core/fetch/ImageResource.cpp

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Waldo Bastian (bastian@kde.org)
     Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
     Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
     License as published by the Free Software Foundation; either
@@ skipping 86 matching lines @@
   MultipartImageResourceParser::Client::trace(visitor);
 }
 
 void ImageResource::checkNotify() {
   notifyObserversInternal(MarkFinishedOption::ShouldMarkFinished);
   Resource::checkNotify();
 }
 
 void ImageResource::notifyObserversInternal(
     MarkFinishedOption markFinishedOption) {
-  if (isLoading())
+  if (isLoading() || isNotifyClientsOfCompletionProhibited())
     return;
 
   for (auto* observer : m_observers.asVector()) {
     if (m_observers.contains(observer)) {
       if (markFinishedOption == MarkFinishedOption::ShouldMarkFinished)
         markObserverFinished(observer);
       observer->imageNotifyFinished(this);
     }
   }
 }
@@ skipping 25 matching lines @@
   // On the other hand, when the response is multipart, |updateImage()| is not
   // called in |appendData()|, which means |m_image| might not be created even
   // when |data()| exists. This is intentional since creating a |m_image| on
   // receiving data might destroy an existing image in a previous part.
   DCHECK((m_multipartParser && isLoading()) || !data() || m_image);
 
   if (m_image && !m_image->isNull()) {
     observer->imageChanged(this);
   }
 
-  if (isLoaded()) {
+  if (isLoaded() && !isNotifyClientsOfCompletionProhibited()) {
     markObserverFinished(observer);
     observer->imageNotifyFinished(this);
   }
 }
 
 void ImageResource::removeObserver(ImageResourceObserver* observer) {
   DCHECK(observer);
 
   if (m_observers.contains(observer))
     m_observers.remove(observer);
@@ skipping 361 matching lines @@
     m_image->setAnimationPolicy(newPolicy);
   }
 }
 
 void ImageResource::reloadIfLoFi(ResourceFetcher* fetcher) {
   if (resourceRequest().loFiState() != WebURLRequest::LoFiOn)
     return;
   if (isLoaded() &&
       !response().httpHeaderField("chrome-proxy").contains("q=low"))
     return;
-  setCachePolicyBypassingCache();
-  setLoFiStateOff();
-  if (isLoading())
-    loader()->cancel();
-  clear();
-  notifyObservers();
 
-  setStatus(NotStarted);
+  {

[Nate Chapin, 2016/10/10 23:47:49]

I don't think you need to force this to have a narrower scope than this
function.

[sclittle, 2016/10/11 02:56:56]

AIUI we only want to prevent completion callbacks from being sent before we
start the reload - we want to allow completion callbacks to be sent after the
reload is started. Any new observers added after the call to
setStatus(NotStarted) won't be immediately notified of completion, so that
should be fine.

The main reason it has a narrower scope is because I'm worried about
fetcher->startLoad(). Right now, I don't know of any way that
fetcher->startLoad() would complete the fetch synchronously (and thus want to
call the completion callbacks because the fetch completed), but it's possible
that a new codepath could be added in the future that does this.

If you think this would be better at the function scope, then I'd be happy to
move it.

[Nate Chapin, 2016/10/12 22:56:17]

If startLoad() is someday allowed to complete the fetch synchronously, there
will be lots of details the author of that change will have to work through.
Feel free to make that hypothetical future author's life more difficult and save
us the complexity in the more likely case that we never allow it :)

[sclittle, 2016/10/12 23:38:16]

Fair enough :) I've removed the dependency on AutoReset, and changed this to
just set the m_isSchedulingReload to true and false directly, which should be
fine since multiple calls to reloadIfLoFi should never be nested anyways.

I'm still resetting m_isSchedulingReload before the call to
fetcher->startLoad(), because there's no extra complexity to do that now, but I
can move it after fetcher->startLoad() if you'd prefer.

+    // Prevent clients and observers from being notified of completion while the
+    // reload is being scheduled, so that e.g. canceling an existing load in
+    // progress doesn't cause clients and observers to be notified of completion
+    // prematurely.
+    Resource::ProhibitNotifyClientsOfCompletionInScope

[Nate Chapin, 2016/10/10 23:47:49]

AutoReset<bool> instead of a custom helper?

[sclittle, 2016/10/11 02:56:56]

I don't see an easy way to do that, unless we want to expose a pointer to
Resource::m_isNotifyClientsOfCompletionProhibited to subclasses. AutoReset
doesn't allow copy or assign, so we can't just return an AutoReset<bool> from a
method.

[Nate Chapin, 2016/10/12 22:56:17]

Let's use AutoReset if and only if we settle on moving
m_isNotifyClientsOfCompletionProhibited to ImageResource, then?

[sclittle, 2016/10/12 23:38:16]

I've removed the dependency on AutoReset and changed this to directly set/reset
the bool, which should be fine because we should never schedule a reload while
scheduling a reload.

+        scopedProhibitNotifyCompletion(this);
+
+    setCachePolicyBypassingCache();
+    setLoFiStateOff();
+    if (isLoading()) {
+      loader()->cancel();

[Nate Chapin, 2016/10/10 23:47:49]

Is this the only case where you want to suppress callbacks? If so, the scoped
suppressor should probably go in this if() block?

[sclittle, 2016/10/11 02:56:56]

loader()->cancel() is the main case I'm worried about, but we don't want other
calls like clear() or notifyObservers() to cause completion callbacks to be sent
either (e.g. if one of the observers notified in notifyObservers() adds a new
client or observer or something).

Only completion callbacks like ImageResourceObserver::imageNotifyFinished() and
ResourceClient::notifyFinished() are suppressed, so other callbacks like
ImageResourceObserver::imageChanged() can still run.

[Nate Chapin, 2016/10/12 22:56:17]

clear() is trivial, but notifyObservers() could have unpredictable side effects.
This scoping should be fine then.

[sclittle, 2016/10/12 23:38:16]

Acknowledged.

+      // Canceling the loader causes error() to be called, which in turn calls
+      // clear() and notifyObservers(), so there's no need to call these again
+      // here.
+    } else {
+      clear();
+      notifyObservers();
+    }
+
+    setStatus(NotStarted);
+  }
+
   fetcher->startLoad(this);
 }
 
 void ImageResource::changedInRect(const blink::Image* image,
                                   const IntRect& rect) {
   if (!image || image != m_image)
     return;
   notifyObservers(&rect);
 }
 
@@ skipping 33 matching lines @@
     return response().serviceWorkerResponseType() !=
            WebServiceWorkerResponseTypeOpaque;
   if (!getImage()->currentFrameHasSingleSecurityOrigin())
     return false;
   if (passesAccessControlCheck(securityOrigin))
     return true;
   return !securityOrigin->taintsCanvas(response().url());
 }
 
 }  // namespace blink