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

third_party/WebKit/Source/core/fetch/Resource.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, 2008, 2009, 2010, 2011 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
@@ skipping 299 matching lines @@
       m_cacheIdentifier(MemoryCache::defaultCacheIdentifier()),
       m_preloadResult(PreloadNotReferenced),
       m_type(type),
       m_status(NotStarted),
       m_needsSynchronousCacheHit(false),
       m_linkPreload(false),
       m_isRevalidating(false),
       m_isAlive(false),
       m_integrityDisposition(ResourceIntegrityDisposition::NotChecked),
       m_isAddRemoveClientProhibited(false),
+      m_isNotifyClientsOfCompletionProhibited(false),

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

I'm suspicious this can be on ImageResource instead of Resource. I think you can
stop all these notifications by hooking ImageResource::checkNotify? Especially
since clients shouldn't get added/removed during cancellation if callbacks are
being surpressed. Is there a case I'm forgetting about?

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

Only the completion callbacks are being suppressed, not others like
ImageResourceObserver::imageChanged(), so IIUC it's possible that an
imageChanged() or other kind of callback gets called and adds a new
ResourceClient.

We could try to suppress imageChanged callbacks too, but there are other
ImageResourceObserver callbacks that probably shouldn't be suppressed during a
reload and could still possibly add clients or observers, like
ImageResourceObserver::computeResourcePriority().

I could be missing something, but AIUI it seems easier to just handle this in
Resource than to enforce that no callbacks add observers or clients. WDYT?

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

I'm not certain how much of a risk this is in practice, but assuming it is: I
think supressing in ImageResource::didAddClient and ImageResource::checkNotify
should case all caseS?

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

Good point, sorry, I forgot about ImageResource::didAddClient. I've moved this
boolean to ImageResource, and renamed it to m_isSchedulingReload to be more
specific.

       m_options(options),
       m_responseTimestamp(currentTime()),
       m_cancelTimer(this, &Resource::cancelTimerFired),
       m_resourceRequest(request) {
   // m_type is a bitfield, so this tests careless updates of the enum.
   DCHECK_EQ(m_type, unsigned(type));
   InstanceCounters::incrementCounter(InstanceCounters::ResourceCounter);
 
   // Currently we support the metadata caching only for HTTP family.
   if (m_resourceRequest.url().protocolIsInHTTPFamily())
@@ skipping 19 matching lines @@
   DCHECK(stillNeedsLoad());
   m_loader = loader;
   m_status = Pending;
 }
 
 void Resource::checkNotify() {
   notifyClientsInternal(MarkFinishedOption::ShouldMarkFinished);
 }
 
 void Resource::notifyClientsInternal(MarkFinishedOption markFinishedOption) {
-  if (isLoading())
+  if (isLoading() || m_isNotifyClientsOfCompletionProhibited)
     return;
 
   ResourceClientWalker<ResourceClient> w(m_clients);
   while (ResourceClient* c = w.next()) {
     if (markFinishedOption == MarkFinishedOption::ShouldMarkFinished)
       markClientFinished(c);
     c->notifyFinished(this);
   }
 }
 
@@ skipping 279 matching lines @@
   }
   if (memoryCache()->contains(this)) {
     if (!builder.isEmpty())
       builder.append(' ');
     builder.append("in_memory_cache");
   }
   return builder.toString();
 }
 
 void Resource::didAddClient(ResourceClient* c) {
-  if (isLoaded()) {
+  if (isLoaded() && !m_isNotifyClientsOfCompletionProhibited) {
     c->notifyFinished(this);
     if (m_clients.contains(c)) {
       m_finishedClients.add(c);
       m_clients.remove(c);
     }
   }
 }
 
 static bool shouldSendCachedDataSynchronouslyForType(Resource::Type type) {
   // Some resources types default to return data synchronously. For most of
@@ skipping 427 matching lines @@
     case Resource::TextTrack:
     case Resource::Media:
     case Resource::Manifest:
       return false;
   }
   NOTREACHED();
   return false;
 }
 
 }  // namespace blink