From 3a9bbe30ac3a45748dbc4796ab4f2f51558be2f2 Mon Sep 17 00:00:00 2001 From: Rob Brackett Date: Mon, 18 Apr 2016 10:33:56 -0700 Subject: [PATCH 1/5] Test for #5183 - webContents.executeJavaScript hangs on subframe load. --- spec/api-browser-window-spec.js | 34 +++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 2a8d1295c7fd..26d4ff4af35c 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -4,6 +4,7 @@ const assert = require('assert') const fs = require('fs') const path = require('path') const os = require('os') +const http = require('http') const remote = require('electron').remote const screen = require('electron').screen @@ -785,6 +786,14 @@ describe('browser-window module', function () { describe('window.webContents.executeJavaScript', function () { var expected = 'hello, world!' var code = '(() => "' + expected + '")()' + var server + + afterEach(function () { + if (server) { + server.close() + server = null + } + }) it('doesnt throw when no calback is provided', function () { const result = ipcRenderer.sendSync('executeJavaScript', code, false) @@ -798,6 +807,31 @@ describe('browser-window module', function () { done() }) }) + + it('works after page load and during subframe load', function (done) { + var url + // a slow server, guaranteeing time to execute code during loading + server = http.createServer(function (req, res) { + setTimeout(function() { res.end('') }, 200) + }); + server.listen(0, '127.0.0.1', function () { + url = 'http://127.0.0.1:' + server.address().port + w.loadURL('file://' + path.join(fixtures, 'pages', 'base-page.html')) + }) + + w.webContents.once('did-finish-load', function() { + // initiate a sub-frame load, then try and execute script during it + w.webContents.executeJavaScript(` + var iframe = document.createElement('iframe') + iframe.src = '${url}' + document.body.appendChild(iframe) + `, function() { + w.webContents.executeJavaScript(`console.log('hello')`, function() { + done() + }) + }) + }) + }) }) describe('deprecated options', function () { From 64a84dee3bdcc81819733fb7f3e266e8e0bb5709 Mon Sep 17 00:00:00 2001 From: Rob Brackett Date: Mon, 18 Apr 2016 10:37:08 -0700 Subject: [PATCH 2/5] =?UTF-8?q?Add=20`isLoadingMainFrame`=20method=20to=20?= =?UTF-8?q?WebContents.=20Also=20switch=20`webContents.executeJavaScript`?= =?UTF-8?q?=20to=20check=20it=20instead=20of=20`isLoading`.=20There=20does?= =?UTF-8?q?n=E2=80=99t=20seem=20to=20be=20a=20reasonable=20public=20way=20?= =?UTF-8?q?to=20get=20this=20information=20out=20of=20Chromium,=20so=20it?= =?UTF-8?q?=E2=80=99s=20synthesized=20here=20based=20on=20WebContentsObser?= =?UTF-8?q?ver=20callbacks.=20Fixes=20#5183.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- atom/browser/api/atom_api_web_contents.cc | 33 +++++++++++++++++++++-- atom/browser/api/atom_api_web_contents.h | 11 ++++++++ lib/browser/api/web-contents.js | 2 +- lib/renderer/web-view/web-view.js | 1 + 4 files changed, 44 insertions(+), 3 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 64a76d57e652..c6943d506e67 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -220,7 +220,8 @@ WebContents::WebContents(content::WebContents* web_contents) embedder_(nullptr), type_(REMOTE), request_id_(0), - background_throttling_(true) { + background_throttling_(true), + is_loading_main_frame_(false) { AttachAsUserData(web_contents); web_contents->SetUserAgentOverride(GetBrowserContext()->GetUserAgent()); } @@ -229,7 +230,8 @@ WebContents::WebContents(v8::Isolate* isolate, const mate::Dictionary& options) : embedder_(nullptr), request_id_(0), - background_throttling_(true) { + background_throttling_(true), + is_loading_main_frame_(false) { // Read options. options.Get("backgroundThrottling", &background_throttling_); @@ -543,12 +545,32 @@ void WebContents::DocumentLoadedInFrame( void WebContents::DidFinishLoad(content::RenderFrameHost* render_frame_host, const GURL& validated_url) { bool is_main_frame = !render_frame_host->GetParent(); + if (is_main_frame) + is_loading_main_frame_ = false; + Emit("did-frame-finish-load", is_main_frame); if (is_main_frame) Emit("did-finish-load"); } +void WebContents::DidStartProvisionalLoadForFrame( + content::RenderFrameHost* render_frame_host, + const GURL& url, + bool is_error_page, + bool is_iframe_srcdoc) { + if (!render_frame_host->GetParent()) + is_loading_main_frame_ = true; +} + +void WebContents::DidCommitProvisionalLoadForFrame( + content::RenderFrameHost* render_frame_host, + const GURL& url, + ui::PageTransition transition_type) { + if (!render_frame_host->GetParent()) + is_loading_main_frame_ = true; +} + void WebContents::DidFailProvisionalLoad( content::RenderFrameHost* render_frame_host, const GURL& url, @@ -556,6 +578,8 @@ void WebContents::DidFailProvisionalLoad( const base::string16& description, bool was_ignored_by_handler) { bool is_main_frame = !render_frame_host->GetParent(); + if (is_main_frame) + is_loading_main_frame_ = false; Emit("did-fail-provisional-load", code, description, url, is_main_frame); Emit("did-fail-load", code, description, url, is_main_frame); } @@ -792,6 +816,10 @@ base::string16 WebContents::GetTitle() const { bool WebContents::IsLoading() const { return web_contents()->IsLoading(); } + +bool WebContents::IsLoadingMainFrame() const { + return is_loading_main_frame_; +} bool WebContents::IsWaitingForResponse() const { return web_contents()->IsWaitingForResponse(); @@ -1189,6 +1217,7 @@ void WebContents::BuildPrototype(v8::Isolate* isolate, .SetMethod("_getURL", &WebContents::GetURL) .SetMethod("getTitle", &WebContents::GetTitle) .SetMethod("isLoading", &WebContents::IsLoading) + .SetMethod("isLoadingMainFrame", &WebContents::IsLoadingMainFrame) .SetMethod("isWaitingForResponse", &WebContents::IsWaitingForResponse) .SetMethod("_stop", &WebContents::Stop) .SetMethod("_goBack", &WebContents::GoBack) diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index 0cb2a348e170..7e4bd693536a 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -62,6 +62,7 @@ class WebContents : public mate::TrackableObject, GURL GetURL() const; base::string16 GetTitle() const; bool IsLoading() const; + bool IsLoadingMainFrame() const; bool IsWaitingForResponse() const; void Stop(); void ReloadIgnoringCache(); @@ -228,6 +229,13 @@ class WebContents : public mate::TrackableObject, int error_code, const base::string16& error_description, bool was_ignored_by_handler) override; + void DidStartProvisionalLoadForFrame(content::RenderFrameHost* render_frame_host, + const GURL& validated_url, + bool is_error_page, + bool is_iframe_srcdoc) override; + void DidCommitProvisionalLoadForFrame(content::RenderFrameHost* render_frame_host, + const GURL& url, + ui::PageTransition transition_type) override; void DidStartLoading() override; void DidStopLoading() override; void DidGetResourceResponseStart( @@ -302,6 +310,9 @@ class WebContents : public mate::TrackableObject, // Whether background throttling is disabled. bool background_throttling_; + // Whether the main frame (not just a sub-frame) is currently loading. + bool is_loading_main_frame_; + DISALLOW_COPY_AND_ASSIGN(WebContents); }; diff --git a/lib/browser/api/web-contents.js b/lib/browser/api/web-contents.js index 40efa77cda02..dfacf6a02cf1 100644 --- a/lib/browser/api/web-contents.js +++ b/lib/browser/api/web-contents.js @@ -116,7 +116,7 @@ let wrapWebContents = function (webContents) { callback = hasUserGesture hasUserGesture = false } - if (this.getURL() && !this.isLoading()) { + if (this.getURL() && !this.isLoadingMainFrame()) { return asyncWebFrameMethods.call(this, requestId, 'executeJavaScript', callback, code, hasUserGesture) } else { return this.once('did-finish-load', asyncWebFrameMethods.bind(this, requestId, 'executeJavaScript', callback, code, hasUserGesture)) diff --git a/lib/renderer/web-view/web-view.js b/lib/renderer/web-view/web-view.js index 974d5c6608d6..20f5f07465b5 100644 --- a/lib/renderer/web-view/web-view.js +++ b/lib/renderer/web-view/web-view.js @@ -335,6 +335,7 @@ var registerWebViewElement = function () { 'loadURL', 'getTitle', 'isLoading', + 'isLoadingMainFrame', 'isWaitingForResponse', 'stop', 'reload', From 942971b01afd6773fce41e5ca3d49270cf88b5ce Mon Sep 17 00:00:00 2001 From: Rob Brackett Date: Tue, 19 Apr 2016 19:20:59 -0700 Subject: [PATCH 3/5] Fix linting errors. --- atom/browser/api/atom_api_web_contents.cc | 2 +- atom/browser/api/atom_api_web_contents.h | 27 +++++++++++++---------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index c6943d506e67..7c7e5896abc1 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -816,7 +816,7 @@ base::string16 WebContents::GetTitle() const { bool WebContents::IsLoading() const { return web_contents()->IsLoading(); } - + bool WebContents::IsLoadingMainFrame() const { return is_loading_main_frame_; } diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index 7e4bd693536a..292ec096088f 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -224,18 +224,21 @@ class WebContents : public mate::TrackableObject, int error_code, const base::string16& error_description, bool was_ignored_by_handler) override; - void DidFailProvisionalLoad(content::RenderFrameHost* render_frame_host, - const GURL& validated_url, - int error_code, - const base::string16& error_description, - bool was_ignored_by_handler) override; - void DidStartProvisionalLoadForFrame(content::RenderFrameHost* render_frame_host, - const GURL& validated_url, - bool is_error_page, - bool is_iframe_srcdoc) override; - void DidCommitProvisionalLoadForFrame(content::RenderFrameHost* render_frame_host, - const GURL& url, - ui::PageTransition transition_type) override; + void DidFailProvisionalLoad( + content::RenderFrameHost* render_frame_host, + const GURL& validated_url, + int error_code, + const base::string16& error_description, + bool was_ignored_by_handler) override; + void DidStartProvisionalLoadForFrame( + content::RenderFrameHost* render_frame_host, + const GURL& validated_url, + bool is_error_page, + bool is_iframe_srcdoc) override; + void DidCommitProvisionalLoadForFrame( + content::RenderFrameHost* render_frame_host, + const GURL& url, + ui::PageTransition transition_type) override; void DidStartLoading() override; void DidStopLoading() override; void DidGetResourceResponseStart( From d3e879cd7f3cc7093ebc9246eaccd2b5857faebb Mon Sep 17 00:00:00 2001 From: Rob Brackett Date: Tue, 19 Apr 2016 22:05:09 -0700 Subject: [PATCH 4/5] Change `WebContents::IsLoadingMainFrame` to compare SiteInstances (per @deepak1556's recommendation) Also updates tests to cover the situation where navigating between pages from the same potential "site" and adds generalized tests for `isLoadingMainFrame()`. --- atom/browser/api/atom_api_web_contents.cc | 34 +++------- atom/browser/api/atom_api_web_contents.h | 23 ++----- spec/api-browser-window-spec.js | 76 +++++++++++++++++------ 3 files changed, 69 insertions(+), 64 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 7c7e5896abc1..e39fa985e26b 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -220,8 +220,7 @@ WebContents::WebContents(content::WebContents* web_contents) embedder_(nullptr), type_(REMOTE), request_id_(0), - background_throttling_(true), - is_loading_main_frame_(false) { + background_throttling_(true) { AttachAsUserData(web_contents); web_contents->SetUserAgentOverride(GetBrowserContext()->GetUserAgent()); } @@ -230,8 +229,7 @@ WebContents::WebContents(v8::Isolate* isolate, const mate::Dictionary& options) : embedder_(nullptr), request_id_(0), - background_throttling_(true), - is_loading_main_frame_(false) { + background_throttling_(true) { // Read options. options.Get("backgroundThrottling", &background_throttling_); @@ -545,32 +543,12 @@ void WebContents::DocumentLoadedInFrame( void WebContents::DidFinishLoad(content::RenderFrameHost* render_frame_host, const GURL& validated_url) { bool is_main_frame = !render_frame_host->GetParent(); - if (is_main_frame) - is_loading_main_frame_ = false; - Emit("did-frame-finish-load", is_main_frame); if (is_main_frame) Emit("did-finish-load"); } -void WebContents::DidStartProvisionalLoadForFrame( - content::RenderFrameHost* render_frame_host, - const GURL& url, - bool is_error_page, - bool is_iframe_srcdoc) { - if (!render_frame_host->GetParent()) - is_loading_main_frame_ = true; -} - -void WebContents::DidCommitProvisionalLoadForFrame( - content::RenderFrameHost* render_frame_host, - const GURL& url, - ui::PageTransition transition_type) { - if (!render_frame_host->GetParent()) - is_loading_main_frame_ = true; -} - void WebContents::DidFailProvisionalLoad( content::RenderFrameHost* render_frame_host, const GURL& url, @@ -578,8 +556,6 @@ void WebContents::DidFailProvisionalLoad( const base::string16& description, bool was_ignored_by_handler) { bool is_main_frame = !render_frame_host->GetParent(); - if (is_main_frame) - is_loading_main_frame_ = false; Emit("did-fail-provisional-load", code, description, url, is_main_frame); Emit("did-fail-load", code, description, url, is_main_frame); } @@ -818,7 +794,11 @@ bool WebContents::IsLoading() const { } bool WebContents::IsLoadingMainFrame() const { - return is_loading_main_frame_; + // Comparing site instances works because Electron always creates a new site + // instance when navigating, regardless of origin. See AtomBrowserClient. + return (web_contents()->GetLastCommittedURL().is_empty() || + web_contents()->GetSiteInstance() != + web_contents()->GetPendingSiteInstance()) && IsLoading(); } bool WebContents::IsWaitingForResponse() const { diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index 292ec096088f..bc2d4106f5f2 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -224,21 +224,11 @@ class WebContents : public mate::TrackableObject, int error_code, const base::string16& error_description, bool was_ignored_by_handler) override; - void DidFailProvisionalLoad( - content::RenderFrameHost* render_frame_host, - const GURL& validated_url, - int error_code, - const base::string16& error_description, - bool was_ignored_by_handler) override; - void DidStartProvisionalLoadForFrame( - content::RenderFrameHost* render_frame_host, - const GURL& validated_url, - bool is_error_page, - bool is_iframe_srcdoc) override; - void DidCommitProvisionalLoadForFrame( - content::RenderFrameHost* render_frame_host, - const GURL& url, - ui::PageTransition transition_type) override; + void DidFailProvisionalLoad(content::RenderFrameHost* render_frame_host, + const GURL& validated_url, + int error_code, + const base::string16& error_description, + bool was_ignored_by_handler) override; void DidStartLoading() override; void DidStopLoading() override; void DidGetResourceResponseStart( @@ -313,9 +303,6 @@ class WebContents : public mate::TrackableObject, // Whether background throttling is disabled. bool background_throttling_; - // Whether the main frame (not just a sub-frame) is currently loading. - bool is_loading_main_frame_; - DISALLOW_COPY_AND_ASSIGN(WebContents); }; diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 26d4ff4af35c..1110269e5de1 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -19,6 +19,23 @@ const isCI = remote.getGlobal('isCi') describe('browser-window module', function () { var fixtures = path.resolve(__dirname, 'fixtures') var w = null + var server + + before(function (done) { + server = http.createServer(function (req, res) { + function respond() { res.end(''); } + setTimeout(respond, req.url.includes('slow') ? 200 : 0) + }); + server.listen(0, '127.0.0.1', function () { + server.url = 'http://127.0.0.1:' + server.address().port + done() + }) + }) + + after(function () { + server.close() + server = null + }) beforeEach(function () { if (w != null) { @@ -635,6 +652,44 @@ describe('browser-window module', function () { assert.equal(w.isResizable(), true) }) }) + + describe('loading main frame state', function () { + it('is true when the main frame is loading', function (done) { + w.webContents.on('did-start-loading', function() { + assert.equal(w.webContents.isLoadingMainFrame(), true) + done() + }) + w.webContents.loadURL(server.url) + }) + + it('is false when only a subframe is loading', function (done) { + w.webContents.once('did-finish-load', function() { + assert.equal(w.webContents.isLoadingMainFrame(), false) + w.webContents.on('did-start-loading', function() { + assert.equal(w.webContents.isLoadingMainFrame(), false) + done() + }) + w.webContents.executeJavaScript(` + var iframe = document.createElement('iframe') + iframe.src = '${server.url}/page2' + document.body.appendChild(iframe) + `) + }) + w.webContents.loadURL(server.url) + }) + + it('is true when navigating to pages from the same origin', function (done) { + w.webContents.once('did-finish-load', function() { + assert.equal(w.webContents.isLoadingMainFrame(), false) + w.webContents.on('did-start-loading', function() { + assert.equal(w.webContents.isLoadingMainFrame(), true) + done() + }) + w.webContents.loadURL(`${server.url}/page2`) + }) + w.webContents.loadURL(server.url) + }) + }) }) describe('window states (excluding Linux)', function () { @@ -786,14 +841,6 @@ describe('browser-window module', function () { describe('window.webContents.executeJavaScript', function () { var expected = 'hello, world!' var code = '(() => "' + expected + '")()' - var server - - afterEach(function () { - if (server) { - server.close() - server = null - } - }) it('doesnt throw when no calback is provided', function () { const result = ipcRenderer.sendSync('executeJavaScript', code, false) @@ -809,21 +856,11 @@ describe('browser-window module', function () { }) it('works after page load and during subframe load', function (done) { - var url - // a slow server, guaranteeing time to execute code during loading - server = http.createServer(function (req, res) { - setTimeout(function() { res.end('') }, 200) - }); - server.listen(0, '127.0.0.1', function () { - url = 'http://127.0.0.1:' + server.address().port - w.loadURL('file://' + path.join(fixtures, 'pages', 'base-page.html')) - }) - w.webContents.once('did-finish-load', function() { // initiate a sub-frame load, then try and execute script during it w.webContents.executeJavaScript(` var iframe = document.createElement('iframe') - iframe.src = '${url}' + iframe.src = '${server.url}/slow' document.body.appendChild(iframe) `, function() { w.webContents.executeJavaScript(`console.log('hello')`, function() { @@ -831,6 +868,7 @@ describe('browser-window module', function () { }) }) }) + w.loadURL(server.url) }) }) From dc8fc7c079ec0eec416eedb0bfd54b68de42b1b8 Mon Sep 17 00:00:00 2001 From: Rob Brackett Date: Tue, 19 Apr 2016 23:27:22 -0700 Subject: [PATCH 5/5] :memo: Add English docs for `webContents.isLoadingMainFrame()` [ci skip] --- docs/api/web-contents.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index 8fc7a959f69d..cb0998804a57 100644 --- a/docs/api/web-contents.md +++ b/docs/api/web-contents.md @@ -353,6 +353,10 @@ Returns the title of the current web page. Returns whether web page is still loading resources. +### `webContents.isLoadingMainFrame()` + +Returns whether the main frame (and not just iframes or frames within it) is still loading. + ### `webContents.isWaitingForResponse()` Returns whether the web page is waiting for a first-response from the main