Revert "Fix race condition when calling JsAsker::BeforeStartInUI"

This reverts commit 37317d74adb53afdcb22c85f2d3987fbae290ac7.

atom/browser/net/js_asker.cc

@@ -17,13 +17,11 @@ namespace {
 
 // The callback which is passed to |handler|.
 void HandlerCallback(const BeforeStartCallback& before_start,
-                     const base::Closure& before_post_callback,
                      const ResponseCallback& callback,
                      mate::Arguments* args) {
   // If there is no argument passed then we failed.
   v8::Local<v8::Value> value;
   if (!args->GetNext(&value)) {
-    before_post_callback.Run();
     content::BrowserThread::PostTask(
         content::BrowserThread::IO, FROM_HERE,
         base::Bind(callback, false, nullptr));
@@ -37,7 +35,6 @@ void HandlerCallback(const BeforeStartCallback& before_start,
   V8ValueConverter converter;
   v8::Local<v8::Context> context = args->isolate()->GetCurrentContext();
   std::unique_ptr<base::Value> options(converter.FromV8Value(value, context));
-  before_post_callback.Run();
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(callback, true, base::Passed(&options)));
@@ -49,7 +46,6 @@ void AskForOptions(v8::Isolate* isolate,
                    const JavaScriptHandler& handler,
                    std::unique_ptr<base::DictionaryValue> request_details,
                    const BeforeStartCallback& before_start,
-                   const base::Closure& before_post_callback,
                    const ResponseCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   v8::Locker locker(isolate);
@@ -59,8 +55,7 @@ void AskForOptions(v8::Isolate* isolate,
   handler.Run(
       *(request_details.get()),
       mate::ConvertToV8(isolate,
-                        base::Bind(&HandlerCallback, before_start,
+                        base::Bind(&HandlerCallback, before_start, callback)));
-                                   before_post_callback, callback)));
 }
 
 bool IsErrorOptions(base::Value* value, int* error) {

atom/browser/net/js_asker.h

@@ -9,7 +9,6 @@
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "base/synchronization/waitable_event.h"
 #include "base/values.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/net_errors.h"
@@ -36,7 +35,6 @@ void AskForOptions(v8::Isolate* isolate,
                    const JavaScriptHandler& handler,
                    std::unique_ptr<base::DictionaryValue> request_details,
                    const BeforeStartCallback& before_start,
-                   const base::Closure& before_post_callback,
                    const ResponseCallback& callback);
 
 // Test whether the |options| means an error.
@@ -48,21 +46,7 @@ template<typename RequestJob>
 class JsAsker : public RequestJob {
  public:
   JsAsker(net::URLRequest* request, net::NetworkDelegate* network_delegate)
-      : RequestJob(request, network_delegate),
+      : RequestJob(request, network_delegate), weak_factory_(this) {}
-        wait_ui_event_(base::WaitableEvent::ResetPolicy::MANUAL,
-                       base::WaitableEvent::InitialState::SIGNALED),
-        weak_factory_(this) {}
-
-  ~JsAsker() override {
-    // It is not safe to destroy JsAsker without calling Kill() while waiting
-    // for response from UI thread. Because by the time ~JsAsker() is called,
-    // the subclass has already been destructed, while we are still executing
-    // the subclass's BeforeStartInUI() method.
-    // But this corner case should never happen, since Kill() is usually called
-    // before destroying the URLRequestJob, so we just assert for that case.
-    CHECK(wait_ui_event_.IsSignaled()) <<
-        "JsAsker is destroyed while waiting for response from UI thread.";
-  }
 
   // Called by |CustomProtocolHandler| to store handler related information.
   void SetHandlerInfo(
@@ -82,38 +66,25 @@ class JsAsker : public RequestJob {
     return request_context_getter_;
   }
 
- protected:
+ private:
   // RequestJob:
   void Start() override {
     std::unique_ptr<base::DictionaryValue> request_details(
         new base::DictionaryValue);
     request_start_time_ = base::TimeTicks::Now();
     FillRequestDetails(request_details.get(), RequestJob::request());
-    // Do not kill the job while waiting for UI thread's response.
-    wait_ui_event_.Reset();
-    // Ask for options from the UI thread.
     content::BrowserThread::PostTask(
         content::BrowserThread::UI, FROM_HERE,
         base::Bind(&internal::AskForOptions,
                    isolate_,
                    handler_,
                    base::Passed(&request_details),
-                   // The RequestJob is guaranteed to exist before OnResponse()
-                   // is called.
                    base::Bind(&JsAsker::BeforeStartInUI,
-                              base::Unretained(this)),
+                              weak_factory_.GetWeakPtr()),
-                   base::Bind(&base::WaitableEvent::Signal,
-                              base::Unretained(&wait_ui_event_)),
                    base::Bind(&JsAsker::OnResponse,
                               weak_factory_.GetWeakPtr())));
   }
 
-  void Kill() override {
-    // Wait for UI thread before killing the job.
-    wait_ui_event_.Wait();
-    RequestJob::Kill();
-  }
-
   int GetResponseCode() const override { return net::HTTP_OK; }
 
   // NOTE: We have to implement this method or risk a crash in blink for
@@ -129,7 +100,6 @@ class JsAsker : public RequestJob {
     info->headers = new net::HttpResponseHeaders("");
   }
 
- private:
   // Called when the JS handler has sent the response, we need to decide whether
   // to start, or fail the job.
   void OnResponse(bool success, std::unique_ptr<base::Value> value) {
@@ -143,12 +113,6 @@ class JsAsker : public RequestJob {
     }
   }
 
-  // Prevent this class from being destroyed when it is still accessed by UI
-  // thread.
-  // FIXME(zcbenz): Refactor JsAsker and its subclasses so this class is not
-  // used by both UI and IO threads.
-  base::WaitableEvent wait_ui_event_;
-
   v8::Isolate* isolate_;
   net::URLRequestContextGetter* request_context_getter_;
   JavaScriptHandler handler_;