fix lifetime of downloadItem class when defaultdownload canceled

diff --git a/atom/browser/api/atom_api_download_item.cc b/atom/browser/api/atom_api_download_item.cc
index f186821..c3beee5 100644 (file)
--- a/atom/browser/api/atom_api_download_item.cc
+++ b/atom/browser/api/atom_api_download_item.cc
@@ -12,6 +12,7 @@
 #include "atom/common/native_mate_converters/gurl_converter.h"
 #include "atom/common/node_includes.h"
 #include "base/memory/linked_ptr.h"
+#include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "native_mate/dictionary.h"
 #include "net/base/filename_util.h"
@@ -70,8 +71,17 @@ DownloadItem::DownloadItem(content::DownloadItem* download_item) :
 }
 
 DownloadItem::~DownloadItem() {
-  if (download_item_)
-    OnDownloadDestroyed(download_item_);
+  Destroy(download_item_);
+}
+
+void DownloadItem::Destroy(content::DownloadItem* item) {
+  if (item) {
+    OnDownloadDestroyed(item);
+    MarkDestroyed();
+
+    // Destroy the native class in next tick.
+    base::MessageLoop::current()->PostTask(FROM_HERE, GetDestroyClosure());
+  }
 }
 
 void DownloadItem::OnDownloadUpdated(content::DownloadItem* item) {
@@ -79,8 +89,8 @@ void DownloadItem::OnDownloadUpdated(content::DownloadItem* item) {
 }
 
 void DownloadItem::OnDownloadDestroyed(content::DownloadItem* download_item) {
-  download_item_->RemoveObserver(this);
-  auto iter = g_download_item_objects.find(download_item_->GetId());
+  download_item->RemoveObserver(this);
+  auto iter = g_download_item_objects.find(download_item->GetId());
   if (iter != g_download_item_objects.end())
     g_download_item_objects.erase(iter);
   download_item_ = nullptr;

diff --git a/atom/browser/api/atom_api_download_item.h b/atom/browser/api/atom_api_download_item.h
index 471913c..af388a5 100644 (file)
--- a/atom/browser/api/atom_api_download_item.h
+++ b/atom/browser/api/atom_api_download_item.h
@@ -36,6 +36,8 @@ class DownloadItem : public mate::TrackableObject<DownloadItem>,
   static void BuildPrototype(v8::Isolate* isolate,
                              v8::Local<v8::ObjectTemplate> prototype);
 
+  void Destroy(content::DownloadItem* download_item);
+
  protected:
   explicit DownloadItem(content::DownloadItem* download_item);
   ~DownloadItem();

diff --git a/atom/browser/api/atom_api_session.cc b/atom/browser/api/atom_api_session.cc
index 37909c5..f0bd739 100644 (file)
--- a/atom/browser/api/atom_api_session.cc
+++ b/atom/browser/api/atom_api_session.cc
@@ -292,12 +292,16 @@ void Session::OnDownloadCreated(content::DownloadManager* manager,
   auto web_contents = item->GetWebContents();
   if (SavePageHandler::IsSavePageTypes(item->GetMimeType()))
     return;
+  auto download_item_handle = DownloadItem::Create(isolate(), item);
   bool prevent_default = Emit(
       "will-download",
-      DownloadItem::Create(isolate(), item),
+      download_item_handle,
       api::WebContents::CreateFrom(isolate(), web_contents));
-  if (prevent_default)
+  if (prevent_default) {
     item->Cancel(true);
+    download_item_handle->Destroy(item);
+    item->Remove();
+  }
 }
 
 void Session::ResolveProxy(const GURL& url, ResolveProxyCallback callback) {

diff --git a/docs/api/session.md b/docs/api/session.md
index 8b0fa4e..255f6a0 100644 (file)
--- a/docs/api/session.md
+++ b/docs/api/session.md
@@ -60,7 +60,8 @@ The following events are available on instances of `Session`:
 
 Emitted when Electron is about to download `item` in `webContents`.
 
-Calling `event.preventDefault()` will cancel the download.
+Calling `event.preventDefault()` will cancel the download and `item` will not be
+available from next tick of the process.
 
 ```javascript
 session.defaultSession.on('will-download', function(event, item, webContents) {

diff --git a/spec/api-session-spec.js b/spec/api-session-spec.js
index 9340204..fe27f93 100644 (file)
--- a/spec/api-session-spec.js
+++ b/spec/api-session-spec.js
@@ -144,6 +144,50 @@ describe('session module', function() {
       });
     });
   });
+
+  describe('session will-download event', function() {
+    var w = null;
+
+    beforeEach(function() {
+      w = new BrowserWindow({
+        show: false,
+        width: 400,
+        height: 400
+      });
+    });
+    afterEach(function() {
+      return w.destroy();
+    });
+
+    it('can cancel default download behavior', function(done) {
+      const mockFile = new Buffer(1024);
+      const contentDisposition = 'inline; filename="mockFile.txt"';
+      const downloadServer = http.createServer(function(req, res) {
+        res.writeHead(200, {
+          'Content-Length': mockFile.length,
+          'Content-Type': 'application/plain',
+          'Content-Disposition': contentDisposition
+        });
+        res.end(mockFile);
+        downloadServer.close();
+      });
+
+      downloadServer.listen(0, '127.0.0.1', function() {
+        const port = downloadServer.address().port;
+        const url = "http://127.0.0.1:" + port + '/';
+
+        ipcRenderer.sendSync('set-download-option', false, true);
+        w.loadURL(url);
+        ipcRenderer.once('download-error', function(event, downloadUrl, filename, error) {
+          assert.equal(downloadUrl, url);
+          assert.equal(filename, 'mockFile.txt');
+          assert.equal(error, 'Object has been destroyed');
+          done();
+        });
+      });
+    });
+  });
+
   return describe('DownloadItem', function() {
     var assertDownload, contentDisposition, downloadFilePath, downloadServer, mockPDF;
     mockPDF = new Buffer(1024 * 1024 * 5);
@@ -173,7 +217,7 @@ describe('session module', function() {
       return downloadServer.listen(0, '127.0.0.1', function() {
         var port;
         port = downloadServer.address().port;
-        ipcRenderer.sendSync('set-download-option', false);
+        ipcRenderer.sendSync('set-download-option', false, false);
         w.loadURL(url + ":" + port);
         return ipcRenderer.once('download-done', function(event, state, url, mimeType, receivedBytes, totalBytes, disposition, filename) {
           assertDownload(event, state, url, mimeType, receivedBytes, totalBytes, disposition, filename, port);
@@ -185,7 +229,7 @@ describe('session module', function() {
       return downloadServer.listen(0, '127.0.0.1', function() {
         var port, webview;
         port = downloadServer.address().port;
-        ipcRenderer.sendSync('set-download-option', false);
+        ipcRenderer.sendSync('set-download-option', false, false);
         webview = new WebView;
         webview.src = "file://" + fixtures + "/api/blank.html";
         webview.addEventListener('did-finish-load', function() {
@@ -199,11 +243,11 @@ describe('session module', function() {
         return document.body.appendChild(webview);
       });
     });
-    return it('can cancel download', function(done) {
+    it('can cancel download', function(done) {
       return downloadServer.listen(0, '127.0.0.1', function() {
         var port;
         port = downloadServer.address().port;
-        ipcRenderer.sendSync('set-download-option', true);
+        ipcRenderer.sendSync('set-download-option', true, false);
         w.loadURL(url + ":" + port + "/");
         return ipcRenderer.once('download-done', function(event, state, url, mimeType, receivedBytes, totalBytes, disposition, filename) {
           assert.equal(state, 'cancelled');

diff --git a/spec/static/main.js b/spec/static/main.js
index 4419a22..13d2dd6 100644 (file)
--- a/spec/static/main.js
+++ b/spec/static/main.js
@@ -102,23 +102,35 @@ app.on('ready', function() {
   // For session's download test, listen 'will-download' event in browser, and
   // reply the result to renderer for verifying
   var downloadFilePath = path.join(__dirname, '..', 'fixtures', 'mock.pdf');
-  ipcMain.on('set-download-option', function(event, need_cancel) {
-    window.webContents.session.once('will-download',
-        function(e, item, webContents) {
-          item.setSavePath(downloadFilePath);
-          item.on('done', function(e, state) {
-            window.webContents.send('download-done',
-                                    state,
-                                    item.getURL(),
-                                    item.getMimeType(),
-                                    item.getReceivedBytes(),
-                                    item.getTotalBytes(),
-                                    item.getContentDisposition(),
-                                    item.getFilename());
-          });
-          if (need_cancel)
-            item.cancel();
+  ipcMain.on('set-download-option', function(event, need_cancel, prevent_default) {
+    window.webContents.session.once('will-download', function(e, item, webContents) {
+      if (prevent_default) {
+        e.preventDefault();
+        const url = item.getURL();
+        const filename = item.getFilename();
+        setImmediate(function() {
+          try {
+            item.getURL();
+          } catch(err) {
+            window.webContents.send('download-error', url, filename, err.message);
+          }
         });
+      } else {
+        item.setSavePath(downloadFilePath);
+        item.on('done', function(e, state) {
+          window.webContents.send('download-done',
+                                  state,
+                                  item.getURL(),
+                                  item.getMimeType(),
+                                  item.getReceivedBytes(),
+                                  item.getTotalBytes(),
+                                  item.getContentDisposition(),
+                                  item.getFilename());
+        });
+        if (need_cancel)
+          item.cancel();
+      }
+    });
     event.returnValue = "done";
   });
 });