From 2819af9586197e592bda2cfb192ad7150839aed9 Mon Sep 17 00:00:00 2001 From: Robo Date: Tue, 2 Feb 2016 15:54:51 +0530 Subject: [PATCH] fix lifetime of downloadItem class when defaultdownload canceled --- atom/browser/api/atom_api_download_item.cc | 18 ++++++++--- atom/browser/api/atom_api_download_item.h | 2 ++ atom/browser/api/atom_api_session.cc | 8 +++-- docs/api/session.md | 3 +- spec/api-session-spec.js | 52 +++++++++++++++++++++++++++--- spec/static/main.js | 44 ++++++++++++++++--------- 6 files changed, 100 insertions(+), 27 deletions(-) diff --git a/atom/browser/api/atom_api_download_item.cc b/atom/browser/api/atom_api_download_item.cc index f186821..c3beee5 100644 --- 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 --- 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, static void BuildPrototype(v8::Isolate* isolate, v8::Local 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 --- 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 --- 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 --- 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 --- 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"; }); }); -- 2.7.4