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

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

Index: third_party/WebKit/Source/core/fetch/ImageResource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ImageResource.cpp b/third_party/WebKit/Source/core/fetch/ImageResource.cpp
index ab9668033457412d10e819243d1f095f2f7e4f19..45f80c728f9763c48e59c5b825e3211cb4ec37f1 100644
--- a/third_party/WebKit/Source/core/fetch/ImageResource.cpp
+++ b/third_party/WebKit/Source/core/fetch/ImageResource.cpp
@@ -104,7 +104,7 @@ void ImageResource::checkNotify() {
 
 void ImageResource::notifyObserversInternal(
     MarkFinishedOption markFinishedOption) {
-  if (isLoading())
+  if (isLoading() || isNotifyClientsOfCompletionProhibited())
     return;
 
   for (auto* observer : m_observers.asVector()) {
@@ -150,7 +150,7 @@ void ImageResource::addObserver(ImageResourceObserver* observer) {
     observer->imageChanged(this);
   }
 
-  if (isLoaded()) {
+  if (isLoaded() && !isNotifyClientsOfCompletionProhibited()) {
     markObserverFinished(observer);
     observer->imageNotifyFinished(this);
   }
@@ -532,14 +532,30 @@ void ImageResource::reloadIfLoFi(ResourceFetcher* fetcher) {
   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);
 }