Fixing a crash with pending URLRequests on shutdown.

atom/browser/api/atom_api_url_request.cc

@@ -132,7 +132,14 @@ URLRequest::URLRequest(v8::Isolate* isolate, v8::Local<v8::Object> wrapper)
   InitWith(isolate, wrapper);
 }
 
-URLRequest::~URLRequest() {}
+URLRequest::~URLRequest() {
+  // A request has been created in JS, it was not used and then
+  // it got collected, no close event to cleanup, only destructor
+  // is called.
+  if (atom_request_) {
+    atom_request_->Terminate();
+  }
+}
 
 // static
 mate::WrappableBase* URLRequest::New(mate::Arguments* args) {
@@ -341,17 +348,15 @@ void URLRequest::OnResponseCompleted() {
   Close();
 }
 
-void URLRequest::OnRequestError(const std::string& error) {
+void URLRequest::OnError(const std::string& error, bool isRequestError) {
   auto error_object = v8::Exception::Error(mate::StringToV8(isolate(), error));
+  if (isRequestError) {
     request_state_.SetFlag(RequestStateFlags::kFailed);
     EmitRequestEvent(false, "error", error_object);
-  Close();
+  } else {
-}
-
-void URLRequest::OnResponseError(const std::string& error) {
-  auto error_object = v8::Exception::Error(mate::StringToV8(isolate(), error));
     response_state_.SetFlag(ResponseStateFlags::kFailed);
     EmitResponseEvent(false, "error", error_object);
+  }
   Close();
 }
 
@@ -398,6 +403,11 @@ void URLRequest::Close() {
     EmitRequestEvent(true, "close");
   }
   unpin();
+  if (atom_request_) {
+    // A request has been created in JS, used and then it ended.
+    // We release unneeded net resources.
+    atom_request_->Terminate();
+  }
   atom_request_ = nullptr;
 }
 

atom/browser/api/atom_api_url_request.h

@@ -104,8 +104,7 @@ class URLRequest : public mate::EventEmitter<URLRequest> {
       scoped_refptr<net::HttpResponseHeaders> response_headers);
   void OnResponseData(scoped_refptr<const net::IOBufferWithSize> data);
   void OnResponseCompleted();
-  void OnRequestError(const std::string& error);
+  void OnError(const std::string& error, bool isRequestError);
-  void OnResponseError(const std::string& error);
 
  protected:
   explicit URLRequest(v8::Isolate* isolate, v8::Local<v8::Object> wrapper);

atom/browser/net/atom_url_request.cc

@@ -48,7 +48,10 @@ AtomURLRequest::AtomURLRequest(base::WeakPtr<api::URLRequest> delegate)
       is_chunked_upload_(false),
       response_read_buffer_(new net::IOBuffer(kBufferSize)) {}
 
-AtomURLRequest::~AtomURLRequest() {}
+AtomURLRequest::~AtomURLRequest() {
+  DCHECK(!request_context_getter_);
+  DCHECK(!request_);
+}
 
 scoped_refptr<AtomURLRequest> AtomURLRequest::Create(
     AtomBrowserContext* browser_context,
@@ -59,11 +62,15 @@ scoped_refptr<AtomURLRequest> AtomURLRequest::Create(
 
   DCHECK(browser_context);
   DCHECK(!url.empty());
-  if (!browser_context || url.empty()) {
+  DCHECK(delegate);
+  if (!browser_context || url.empty() || !delegate) {
     return nullptr;
   }
-
   auto request_context_getter = browser_context->url_request_context_getter();
+  DCHECK(request_context_getter);
+  if (!request_context_getter) {
+    return nullptr;
+  }
   scoped_refptr<AtomURLRequest> atom_url_request(new AtomURLRequest(delegate));
   if (content::BrowserThread::PostTask(
           content::BrowserThread::IO, FROM_HERE,
@@ -74,6 +81,13 @@ scoped_refptr<AtomURLRequest> AtomURLRequest::Create(
   return nullptr;
 }
 
+void AtomURLRequest::Terminate() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&AtomURLRequest::DoTerminate, this));
+}
+
 void AtomURLRequest::DoInitialize(
     scoped_refptr<net::URLRequestContextGetter> request_context_getter,
     const std::string& method,
@@ -81,15 +95,34 @@ void AtomURLRequest::DoInitialize(
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(request_context_getter);
 
-  auto context = request_context_getter->GetURLRequestContext();
+  request_context_getter_ = request_context_getter;
+  request_context_getter_->AddObserver(this);
+  auto context = request_context_getter_->GetURLRequestContext();
+  if (!context) {
+    // Called after shutdown.
+    DoCancelWithError("Cannot start a request after shutdown.", true);
+    return;
+  }
 
   DCHECK(context);
   request_ = context->CreateRequest(
       GURL(url), net::RequestPriority::DEFAULT_PRIORITY, this);
-
+  if (!request_) {
+    DoCancelWithError("Failed to create a net::URLRequest.", true);
+    return;
+  }
   request_->set_method(method);
 }
 
+void AtomURLRequest::DoTerminate() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  request_.reset();
+  if (request_context_getter_) {
+    request_context_getter_->RemoveObserver(this);
+    request_context_getter_ = nullptr;
+  }
+}
+
 bool AtomURLRequest::Write(scoped_refptr<const net::IOBufferWithSize> buffer,
                            bool is_last) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -107,7 +140,7 @@ void AtomURLRequest::SetChunkedUpload(bool is_chunked_upload) {
   is_chunked_upload_ = is_chunked_upload;
 }
 
-void AtomURLRequest::Cancel() const {
+void AtomURLRequest::Cancel() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
                                    base::Bind(&AtomURLRequest::DoCancel, this));
@@ -147,6 +180,9 @@ void AtomURLRequest::DoWriteBuffer(
     scoped_refptr<const net::IOBufferWithSize> buffer,
     bool is_last) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  if (!request_) {
+    return;
+  }
 
   if (is_chunked_upload_) {
     // Chunked encoding case.
@@ -191,32 +227,56 @@ void AtomURLRequest::DoWriteBuffer(
   }
 }
 
-void AtomURLRequest::DoCancel() const {
+void AtomURLRequest::DoCancel() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  if (request_) {
     request_->Cancel();
   }
+  DoTerminate();
+}
 
 void AtomURLRequest::DoSetExtraHeader(const std::string& name,
                                       const std::string& value) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  if (!request_) {
+    return;
+  }
   request_->SetExtraRequestHeaderByName(name, value, true);
 }
 void AtomURLRequest::DoRemoveExtraHeader(const std::string& name) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  if (!request_) {
+    return;
+  }
   request_->RemoveRequestHeaderByName(name);
 }
 
 void AtomURLRequest::DoSetAuth(const base::string16& username,
                                const base::string16& password) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  if (!request_) {
+    return;
+  }
   request_->SetAuth(net::AuthCredentials(username, password));
 }
 
 void AtomURLRequest::DoCancelAuth() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  if (!request_) {
+    return;
+  }
   request_->CancelAuth();
 }
 
+void AtomURLRequest::DoCancelWithError(const std::string& error,
+                                       bool isRequestError) {
+  DoCancel();
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&AtomURLRequest::InformDelegateErrorOccured, this, error,
+                 isRequestError));
+}
+
 void AtomURLRequest::OnAuthRequired(net::URLRequest* request,
                                     net::AuthChallengeInfo* auth_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
@@ -229,6 +289,9 @@ void AtomURLRequest::OnAuthRequired(net::URLRequest* request,
 
 void AtomURLRequest::OnResponseStarted(net::URLRequest* request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  if (!request_) {
+    return;
+  }
   DCHECK_EQ(request, request_.get());
 
   scoped_refptr<net::HttpResponseHeaders> response_headers =
@@ -240,16 +303,10 @@ void AtomURLRequest::OnResponseStarted(net::URLRequest* request) {
         content::BrowserThread::UI, FROM_HERE,
         base::Bind(&AtomURLRequest::InformDelegateResponseStarted, this,
                    response_headers));
-
     ReadResponse();
   } else if (status.status() == net::URLRequestStatus::Status::FAILED) {
     // Report error on Start.
-    DoCancel();
+    DoCancelWithError(net::ErrorToString(status.ToNetError()), true);
-    auto error = net::ErrorToString(status.ToNetError());
-    content::BrowserThread::PostTask(
-        content::BrowserThread::UI, FROM_HERE,
-        base::Bind(&AtomURLRequest::InformDelegateRequestErrorOccured, this,
-                   std::move(error)));
   }
   // We don't report an error is the request is canceled.
 }
@@ -265,7 +322,9 @@ void AtomURLRequest::ReadResponse() {
 
 void AtomURLRequest::OnReadCompleted(net::URLRequest* request, int bytes_read) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-
+  if (!request_) {
+    return;
+  }
   DCHECK_EQ(request, request_.get());
 
   const auto status = request_->status();
@@ -289,26 +348,23 @@ void AtomURLRequest::OnReadCompleted(net::URLRequest* request, int bytes_read) {
   } while (
       request_->Read(response_read_buffer_.get(), kBufferSize, &bytes_read));
   if (response_error) {
-    DoCancel();
+    DoCancelWithError(net::ErrorToString(status.ToNetError()), false);
-    auto error = net::ErrorToString(status.ToNetError());
-    content::BrowserThread::PostTask(
-        content::BrowserThread::UI, FROM_HERE,
-        base::Bind(&AtomURLRequest::InformDelegateResponseErrorOccured, this,
-                   std::move(error)));
   } else if (data_ended) {
     content::BrowserThread::PostTask(
         content::BrowserThread::UI, FROM_HERE,
         base::Bind(&AtomURLRequest::InformDelegateResponseCompleted, this));
+    DoTerminate();
   } else if (data_transfer_error) {
     // We abort the request on corrupted data transfer.
-    DoCancel();
+    DoCancelWithError("Failed to transfer data from IO to UI thread.", false);
-    content::BrowserThread::PostTask(
-        content::BrowserThread::UI, FROM_HERE,
-        base::Bind(&AtomURLRequest::InformDelegateResponseErrorOccured, this,
-                   "Failed to transfer data from IO to UI thread."));
   }
 }
 
+void AtomURLRequest::OnContextShuttingDown() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  DoCancel();
+}
+
 bool AtomURLRequest::CopyAndPostBuffer(int bytes_read) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
@@ -354,20 +410,12 @@ void AtomURLRequest::InformDelegateResponseCompleted() const {
     delegate_->OnResponseCompleted();
 }
 
-void AtomURLRequest::InformDelegateRequestErrorOccured(
+void AtomURLRequest::InformDelegateErrorOccured(const std::string& error,
-    const std::string& error) const {
+                                                bool isRequestError) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   if (delegate_)
-    delegate_->OnRequestError(error);
+    delegate_->OnError(error, isRequestError);
-}
-
-void AtomURLRequest::InformDelegateResponseErrorOccured(
-    const std::string& error) const {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  if (delegate_)
-    delegate_->OnResponseError(error);
 }
 
 }  // namespace atom

atom/browser/net/atom_url_request.h

@@ -18,21 +18,24 @@
 #include "net/base/upload_element_reader.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context_getter_observer.h"
 
 namespace atom {
 
 class AtomURLRequest : public base::RefCountedThreadSafe<AtomURLRequest>,
-                       public net::URLRequest::Delegate {
+                       public net::URLRequest::Delegate,
+                       public net::URLRequestContextGetterObserver {
  public:
   static scoped_refptr<AtomURLRequest> Create(
       AtomBrowserContext* browser_context,
       const std::string& method,
       const std::string& url,
       base::WeakPtr<api::URLRequest> delegate);
+  void Terminate();
 
   bool Write(scoped_refptr<const net::IOBufferWithSize> buffer, bool is_last);
   void SetChunkedUpload(bool is_chunked_upload);
-  void Cancel() const;
+  void Cancel();
   void SetExtraHeader(const std::string& name, const std::string& value) const;
   void RemoveExtraHeader(const std::string& name) const;
   void PassLoginInformation(const base::string16& username,
@@ -45,6 +48,9 @@ class AtomURLRequest : public base::RefCountedThreadSafe<AtomURLRequest>,
   void OnResponseStarted(net::URLRequest* request) override;
   void OnReadCompleted(net::URLRequest* request, int bytes_read) override;
 
+  // Overrides of net::URLRequestContextGetterObserver
+  void OnContextShuttingDown() override;
+
  private:
   friend class base::RefCountedThreadSafe<AtomURLRequest>;
 
@@ -54,15 +60,17 @@ class AtomURLRequest : public base::RefCountedThreadSafe<AtomURLRequest>,
   void DoInitialize(scoped_refptr<net::URLRequestContextGetter>,
                     const std::string& method,
                     const std::string& url);
+  void DoTerminate();
   void DoWriteBuffer(scoped_refptr<const net::IOBufferWithSize> buffer,
                      bool is_last);
-  void DoCancel() const;
+  void DoCancel();
   void DoSetExtraHeader(const std::string& name,
                         const std::string& value) const;
   void DoRemoveExtraHeader(const std::string& name) const;
   void DoSetAuth(const base::string16& username,
                  const base::string16& password) const;
   void DoCancelAuth() const;
+  void DoCancelWithError(const std::string& error, bool isRequestError);
 
   void ReadResponse();
   bool CopyAndPostBuffer(int bytes_read);
@@ -74,11 +82,12 @@ class AtomURLRequest : public base::RefCountedThreadSafe<AtomURLRequest>,
   void InformDelegateResponseData(
       scoped_refptr<net::IOBufferWithSize> data) const;
   void InformDelegateResponseCompleted() const;
-  void InformDelegateRequestErrorOccured(const std::string& error) const;
+  void InformDelegateErrorOccured(const std::string& error,
-  void InformDelegateResponseErrorOccured(const std::string& error) const;
+                                  bool isRequestError) const;
 
   base::WeakPtr<api::URLRequest> delegate_;
   std::unique_ptr<net::URLRequest> request_;
+  scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
 
   bool is_chunked_upload_;
   std::unique_ptr<net::ChunkedUploadDataStream> chunked_stream_;