Make the WebURLLoaderClients in the pepper code destroy (or have some guard) the loader in didFail()

components/nacl/renderer/manifest_downloader.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/nacl/renderer/manifest_downloader.h"
 
 #include "base/callback.h"
 #include "components/nacl/renderer/histogram.h"
 #include "components/nacl/renderer/nexe_load_manager.h"
 #include "net/base/net_errors.h"
@@ skipping 36 matching lines @@
     int encoded_data_length) {
   if (buffer_.size() + data_length > kNaClManifestMaxFileBytes) {
     pp_nacl_error_ = PP_NACL_ERROR_MANIFEST_TOO_LARGE;
     buffer_.clear();
   }
 
   if (pp_nacl_error_ == PP_NACL_ERROR_LOAD_SUCCESS)
     buffer_.append(data, data_length);
 }
 
-void ManifestDownloader::didFinishLoading(
-    blink::WebURLLoader* loader,
-    double finish_time,
-    int64_t total_encoded_data_length) {
+void ManifestDownloader::done() {
   // We log the status code here instead of in didReceiveResponse so that we
   // always log a histogram value, even when we never receive a status code.
   HistogramHTTPStatusCode(
       is_installed_ ? "NaCl.HttpStatusCodeClass.Manifest.InstalledApp" :
                       "NaCl.HttpStatusCodeClass.Manifest.NotInstalledApp",
       status_code_);
 
   cb_.Run(pp_nacl_error_, buffer_);
   delete this;
 }
 
+void ManifestDownloader::didFinishLoading(
+    blink::WebURLLoader* loader,
+    double finish_time,
+    int64_t total_encoded_data_length) {
+  done();

[hiroshige, 2015/08/03 07:27:42]

Since didFinishLoading() is still called before the ThreadableLoader fix is
landed, does simply calling done() cause use-after-free?
(done() called from didFail() deletes |this|)

[tyoshino (SeeGerritForStatus), 2015/08/03 07:59:05]

I thought it's fine as deleting this also deletes url_loader_ to kill the loader
so that the loader gets removed from the resource.

- RawResource::didAddClient() doesn't continue to call Resource::didAddClient()
(which contains notifyFinished() call) when hasClient() returns false.
- Resource::checkNotify() uses ResourceClientWalker not to call clients that
have been already removed.

+}
+
 void ManifestDownloader::didFail(
     blink::WebURLLoader* loader,
     const blink::WebURLError& error) {
   // TODO(teravest): Find a place to share this code with PepperURLLoaderHost.
   pp_nacl_error_ = PP_NACL_ERROR_MANIFEST_LOAD_URL;
   if (error.domain.equals(blink::WebString::fromUTF8(net::kErrorDomain))) {
     switch (error.reason) {
       case net::ERR_ACCESS_DENIED:
       case net::ERR_NETWORK_ACCESS_DENIED:
         pp_nacl_error_ = PP_NACL_ERROR_MANIFEST_NOACCESS_URL;
         break;
     }
   } else {
     // It's a WebKit error.
     pp_nacl_error_ = PP_NACL_ERROR_MANIFEST_NOACCESS_URL;
   }
+
+  done();
 }
 
 }  // namespace nacl