Fix memory corruption related to load blocking resource move

third_party/WebKit/Source/core/fetch/ResourceFetcher.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) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All
     rights reserved.
     Copyright (C) 2009 Torch Mobile Inc. http://www.torchmobile.com/
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
@@ skipping 433 matching lines @@
   return resource;
 }
 
 void ResourceFetcher::moveCachedNonBlockingResourceToBlocking(
     Resource* resource,
     const FetchRequest& request) {
   // TODO(yoav): Test that non-blocking resources (video/audio/track) continue
   // to not-block even after being preloaded and discovered.
   if (resource && resource->loader() &&
       resource->isLoadEventBlockingResourceType() &&
+      m_nonBlockingLoaders.contains(resource->loader()) &&
       resource->isLinkPreload() && !request.forPreload()) {
     m_nonBlockingLoaders.remove(resource->loader());
     m_loaders.add(resource->loader());
   }
 }
 
 void ResourceFetcher::updateMemoryCacheStats(Resource* resource,
                                              RevalidationPolicy policy,
                                              const FetchRequest& request,
                                              const ResourceFactory& factory,
@@ skipping 809 matching lines @@
 
 void ResourceFetcher::acceptDataFromThreadedReceiver(unsigned long identifier,
                                                      const char* data,
                                                      int dataLength,
                                                      int encodedDataLength) {
   context().dispatchDidReceiveData(identifier, data, dataLength,
                                    encodedDataLength);
 }
 
 void ResourceFetcher::moveResourceLoaderToNonBlocking(ResourceLoader* loader) {
+  DCHECK(loader);
   m_nonBlockingLoaders.add(loader);
   m_loaders.remove(loader);

[bokan, 2016/11/30 14:52:42]

Should we check that loader is in m_loaders here too? Maybe at least a DCHECK?

[Yoav Weiss, 2016/11/30 16:11:42]

That makes sense. Not sure how to exercise that code with an indirect test...

[Charlie Harrison, 2016/11/30 16:51:33]

I think we can DCHECK, afaict.

[bokan, 2016/11/30 17:01:22]

I would DCHECK and add an early return since otherwise we end up hanging the
renderer in shipped builds that don't DCHECK. Here and the other DCHECKs.

 }
 
 bool ResourceFetcher::startLoad(Resource* resource) {
   DCHECK(resource);
   DCHECK(resource->stillNeedsLoad());
   if (!context().shouldLoadNewResource(resource->getType())) {
     memoryCache()->remove(resource);
     return false;
   }
 
@@ skipping 22 matching lines @@
   storeResourceTimingInitiatorInformation(resource);
   resource->setFetcherSecurityOrigin(sourceOrigin);
 
   loader->activateCacheAwareLoadingIfNeeded(request);
   loader->start(request, context().loadingTaskRunner(),
                 context().defersLoading());
   return true;
 }
 
 void ResourceFetcher::removeResourceLoader(ResourceLoader* loader) {
+  DCHECK(loader);
   if (m_loaders.contains(loader))
     m_loaders.remove(loader);
   else if (m_nonBlockingLoaders.contains(loader))
     m_nonBlockingLoaders.remove(loader);
   else
     NOTREACHED();
 }
 
 void ResourceFetcher::stopFetching() {
   HeapVector<Member<ResourceLoader>> loadersToCancel;
@@ skipping 304 matching lines @@
   visitor->trace(m_context);
   visitor->trace(m_archive);
   visitor->trace(m_loaders);
   visitor->trace(m_nonBlockingLoaders);
   visitor->trace(m_documentResources);
   visitor->trace(m_preloads);
   visitor->trace(m_resourceTimingInfoMap);
 }
 
 }  // namespace blink