Skip to content

GitLab

T
tangled
Commit 221c4beb authored by fsamuel@chromium.org's avatar fsamuel@chromium.org
Browse files
Options

Browser Plugin: Navigating to the same URL after crash should load a new guest 

This patch contains a few other small changes in behavior:

1. Removing the src attribute after navigation in <webview> will immediately restore it.
2. browserPlugin.terminate() will not issue an IPC as long as the BrowserPlugin is in a crashed state.
3. WebViewTest.Shim was misbehaving because it had lingering <webview>s in the DOM all in the same partition and so terminating one webview terminated them all, and caused multiple exit listeners to fire. This was fixed.
4. Fixed a potential infinite loop that could happen if the src attribute is modified while in a crashed state.

BUG=175206
Test=WebViewTest.Shim: webViewAssignSrcAfterCrash, webViewRemoveSrcAttribute

Review URL: https://chromiumcodereview.appspot.com/12224094

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@182278 0039d316-1c4b-4281-b951-d872f2087c98
parent 97c07947
Hide whitespace changes
Inline Side-by-side
Showing with 91 additions and 21 deletions
chrome/renderer/resources/extensions/web_view.js
View file @ 221c4beb
...@@ -156,8 +156,17 @@ WebView.prototype.handleObjectMutation_ = function(mutation) { ...@@ -156,8 +156,17 @@ WebView.prototype.handleObjectMutation_ = function(mutation) {
this.node_.removeAttribute(mutation.attributeName); this.node_.removeAttribute(mutation.attributeName);
} else { } else {
// Update the <webview> attribute to match the BrowserPlugin attribute. // Update the <webview> attribute to match the BrowserPlugin attribute.
this.node_.setAttribute(mutation.attributeName, // Note: Calling setAttribute on <webview> will trigger its mutation
this.objectNode_.getAttribute(mutation.attributeName)); // observer which will then propagate that attribute to BrowserPlugin. In
// cases where we permit assigning a BrowserPlugin attribute the same value
// again (such as navigation when crashed), this could end up in an infinite
// loop. Thus, we avoid this loop by only updating the <webview> attribute
// if the BrowserPlugin attributes differs from it.
var oldValue = this.node_.getAttribute(mutation.attributeName);
var newValue = this.objectNode_.getAttribute(mutation.attributeName);
if (newValue != oldValue) {
this.node_.setAttribute(mutation.attributeName, newValue);
}
} }
}; };
......
chrome/test/data/extensions/platform_apps/web_view/shim/main.js
View file @ 221c4beb
...@@ -9,13 +9,14 @@ var util = {}; ...@@ -9,13 +9,14 @@ var util = {};
// sure that the <object> shim is created (asynchronously at this point) for the // sure that the <object> shim is created (asynchronously at this point) for the
// <webview> tag. This makes the <webview> tag ready for add/removeEventListener // <webview> tag. This makes the <webview> tag ready for add/removeEventListener
// calls. // calls.
util.createWebViewTagInDOM = function() { util.createWebViewTagInDOM = function(partitionName) {
var webview = document.createElement('webview'); var webview = document.createElement('webview');
webview.style.width = '300px'; webview.style.width = '300px';
webview.style.height = '200px'; webview.style.height = '200px';
document.body.appendChild(webview);
var urlDummy = 'data:text/html,<body>Initial dummy guest</body>'; var urlDummy = 'data:text/html,<body>Initial dummy guest</body>';
webview.setAttribute('src', urlDummy); webview.setAttribute('src', urlDummy);
webview.setAttribute('partition', partitionName);
document.body.appendChild(webview);
return webview; return webview;
}; };
...@@ -69,6 +70,7 @@ onload = function() { ...@@ -69,6 +70,7 @@ onload = function() {
'terminate' 'terminate'
]; ];
var webview = document.createElement('webview'); var webview = document.createElement('webview');
webview.setAttribute('partition', arguments.callee.name);
webview.addEventListener('loadstop', function(e) { webview.addEventListener('loadstop', function(e) {
for (var i = 0; i < apiMethodsToCheck.length; ++i) { for (var i = 0; i < apiMethodsToCheck.length; ++i) {
chrome.test.assertEq('function', chrome.test.assertEq('function',
...@@ -79,7 +81,6 @@ onload = function() { ...@@ -79,7 +81,6 @@ onload = function() {
chrome.test.assertEq('object', typeof webview.contentWindow); chrome.test.assertEq('object', typeof webview.contentWindow);
chrome.test.assertEq('function', chrome.test.assertEq('function',
typeof webview.contentWindow.postMessage); typeof webview.contentWindow.postMessage);
chrome.test.succeed(); chrome.test.succeed();
}); });
webview.setAttribute('src', 'data:text/html,webview check api'); webview.setAttribute('src', 'data:text/html,webview check api');
...@@ -88,8 +89,7 @@ onload = function() { ...@@ -88,8 +89,7 @@ onload = function() {
function webViewEventName() { function webViewEventName() {
var webview = document.createElement('webview'); var webview = document.createElement('webview');
webview.setAttribute('src', 'data:text/html,webview check api'); webview.setAttribute('partition', arguments.callee.name);
document.body.appendChild(webview);
webview.addEventListener('loadstart', function(evt) { webview.addEventListener('loadstart', function(evt) {
chrome.test.assertEq('loadstart', evt.type); chrome.test.assertEq('loadstart', evt.type);
...@@ -106,6 +106,7 @@ onload = function() { ...@@ -106,6 +106,7 @@ onload = function() {
}); });
webview.setAttribute('src', 'data:text/html,trigger navigation'); webview.setAttribute('src', 'data:text/html,trigger navigation');
document.body.appendChild(webview);
}, },
// This test registers two listeners on an event (loadcommit) and removes // This test registers two listeners on an event (loadcommit) and removes
...@@ -113,7 +114,7 @@ onload = function() { ...@@ -113,7 +114,7 @@ onload = function() {
// Current expected behavior is that the second event listener will still // Current expected behavior is that the second event listener will still
// fire without crashing. // fire without crashing.
function webviewDestroyOnEventListener() { function webviewDestroyOnEventListener() {
var webview = util.createWebViewTagInDOM(); var webview = util.createWebViewTagInDOM(arguments.callee.name);
var url = 'data:text/html,<body>Destroy test</body>'; var url = 'data:text/html,<body>Destroy test</body>';
var loadCommitCount = 0; var loadCommitCount = 0;
...@@ -123,8 +124,6 @@ onload = function() { ...@@ -123,8 +124,6 @@ onload = function() {
return; return;
++loadCommitCount; ++loadCommitCount;
if (loadCommitCount == 1) { if (loadCommitCount == 1) {
webview.parentNode.removeChild(webview);
webview = null;
setTimeout(function() { setTimeout(function() {
chrome.test.succeed(); chrome.test.succeed();
}, 0); }, 0);
...@@ -135,6 +134,7 @@ onload = function() { ...@@ -135,6 +134,7 @@ onload = function() {
// The test starts from here, by setting the src to |url|. // The test starts from here, by setting the src to |url|.
webview.addEventListener('loadcommit', function(e) { webview.addEventListener('loadcommit', function(e) {
webview.parentNode.removeChild(webview);
loadCommitCommon(e); loadCommitCommon(e);
}); });
webview.addEventListener('loadcommit', function(e) { webview.addEventListener('loadcommit', function(e) {
...@@ -147,7 +147,7 @@ onload = function() { ...@@ -147,7 +147,7 @@ onload = function() {
// Each of the listener tries to change some properties on the event param, // Each of the listener tries to change some properties on the event param,
// which should not be possible. // which should not be possible.
function cannotMutateEventName() { function cannotMutateEventName() {
var webview = util.createWebViewTagInDOM(); var webview = util.createWebViewTagInDOM(arguments.callee.name);
var url = 'data:text/html,<body>Two</body>'; var url = 'data:text/html,<body>Two</body>';
var loadCommitACalled = false; var loadCommitACalled = false;
...@@ -195,6 +195,7 @@ onload = function() { ...@@ -195,6 +195,7 @@ onload = function() {
// been set raises an exception. // been set raises an exception.
function partitionRaisesException() { function partitionRaisesException() {
var webview = document.createElement('webview'); var webview = document.createElement('webview');
webview.setAttribute('partition', arguments.callee.name);
webview.setAttribute('src', 'data:text/html,trigger navigation'); webview.setAttribute('src', 'data:text/html,trigger navigation');
document.body.appendChild(webview); document.body.appendChild(webview);
setTimeout(function() { setTimeout(function() {
...@@ -209,6 +210,7 @@ onload = function() { ...@@ -209,6 +210,7 @@ onload = function() {
function webViewExecuteScript() { function webViewExecuteScript() {
var webview = document.createElement('webview'); var webview = document.createElement('webview');
webview.setAttribute('partition', arguments.callee.name);
webview.addEventListener('loadstop', function() { webview.addEventListener('loadstop', function() {
webview.executeScript( webview.executeScript(
{code:'document.body.style.backgroundColor = "red";'}, {code:'document.body.style.backgroundColor = "red";'},
...@@ -225,11 +227,8 @@ onload = function() { ...@@ -225,11 +227,8 @@ onload = function() {
// This test calls terminate() on guest after it has already been // This test calls terminate() on guest after it has already been
// terminated. This makes sure we ignore the call gracefully. // terminated. This makes sure we ignore the call gracefully.
function webViewTerminateAfterExitDoesntCrash() { function webViewTerminateAfterExitDoesntCrash() {
var webview = document.querySelector('webview'); var webview = document.createElement('webview');
webview.addEventListener('loadstart', function(evt) { webview.setAttribute('partition', arguments.callee.name);
chrome.test.assertEq('loadstart', evt.type);
});
var loadstopSucceedsTest = false; var loadstopSucceedsTest = false;
webview.addEventListener('loadstop', function(evt) { webview.addEventListener('loadstop', function(evt) {
chrome.test.assertEq('loadstop', evt.type); chrome.test.assertEq('loadstop', evt.type);
...@@ -255,6 +254,47 @@ onload = function() { ...@@ -255,6 +254,47 @@ onload = function() {
}); });
webview.setAttribute('src', 'data:text/html,test terminate() crash.'); webview.setAttribute('src', 'data:text/html,test terminate() crash.');
document.body.appendChild(webview);
},
// This test verifies that assigning the src attribute the same value it had
// prior to a crash spawns off a new guest process.
function webViewAssignSrcAfterCrash() {
var webview = document.createElement('webview');
webview.setAttribute('partition', arguments.callee.name);
var terminated = false;
webview.addEventListener('loadstop', function(evt) {
if (!terminated) {
webview.terminate();
return;
}
// The guest has recovered after being terminated.
chrome.test.succeed();
});
webview.addEventListener('exit', function(evt) {
terminated = true;
webview.setAttribute('src', 'data:text/html,test page');
});
webview.setAttribute('src', 'data:text/html,test page');
document.body.appendChild(webview);
},
// This test verifies that <webview> restores the src attribute if it is
// removed after navigation.
function webViewRemoveSrcAttribute() {
var dataUrl = 'data:text/html,test page';
var webview = document.createElement('webview');
webview.setAttribute('partition', arguments.callee.name);
var terminated = false;
webview.addEventListener('loadstop', function(evt) {
webview.removeAttribute('src');
setTimeout(function() {
chrome.test.assertEq(dataUrl, webview.getAttribute('src'));
chrome.test.succeed();
}, 0);
});
webview.setAttribute('src', dataUrl);
document.body.appendChild(webview);
} }
]); ]);
}; };
content/renderer/browser_plugin/browser_plugin.cc
View file @ 221c4beb
...@@ -413,6 +413,11 @@ void BrowserPlugin::OnGuestContentWindowReady(int instance_id, ...@@ -413,6 +413,11 @@ void BrowserPlugin::OnGuestContentWindowReady(int instance_id,
} }
void BrowserPlugin::OnGuestGone(int instance_id, int process_id, int status) { void BrowserPlugin::OnGuestGone(int instance_id, int process_id, int status) {
// Set the BrowserPlugin in a crashed state before firing event listeners so
// that operations on the BrowserPlugin within listeners are aware that
// BrowserPlugin is in a crashed state.
guest_crashed_ = true;
// We fire the event listeners before painting the sad graphic to give the // We fire the event listeners before painting the sad graphic to give the
// developer an opportunity to display an alternative overlay image on crash. // developer an opportunity to display an alternative overlay image on crash.
std::string termination_status = TerminationStatusToString( std::string termination_status = TerminationStatusToString(
...@@ -427,7 +432,6 @@ void BrowserPlugin::OnGuestGone(int instance_id, int process_id, int status) { ...@@ -427,7 +432,6 @@ void BrowserPlugin::OnGuestGone(int instance_id, int process_id, int status) {
// but leave other member variables valid below. // but leave other member variables valid below.
TriggerEvent(browser_plugin::kEventExit, &props); TriggerEvent(browser_plugin::kEventExit, &props);
guest_crashed_ = true;
// We won't paint the contents of the current backing store again so we might // We won't paint the contents of the current backing store again so we might
// as well toss it out and save memory. // as well toss it out and save memory.
backing_store_.reset(); backing_store_.reset();
...@@ -468,9 +472,9 @@ void BrowserPlugin::OnLoadCommit( ...@@ -468,9 +472,9 @@ void BrowserPlugin::OnLoadCommit(
// If the guest has just committed a new navigation then it is no longer // If the guest has just committed a new navigation then it is no longer
// crashed. // crashed.
guest_crashed_ = false; guest_crashed_ = false;
if (params.is_top_level) { if (params.is_top_level)
UpdateDOMAttribute(browser_plugin::kAttributeSrc, params.url.spec()); UpdateDOMAttribute(browser_plugin::kAttributeSrc, params.url.spec());
}
guest_process_id_ = params.process_id; guest_process_id_ = params.process_id;
guest_route_id_ = params.route_id; guest_route_id_ = params.route_id;
current_nav_entry_index_ = params.current_entry_index; current_nav_entry_index_ = params.current_entry_index;
...@@ -797,7 +801,7 @@ void BrowserPlugin::Go(int relative_index) { ...@@ -797,7 +801,7 @@ void BrowserPlugin::Go(int relative_index) {
} }
void BrowserPlugin::TerminateGuest() { void BrowserPlugin::TerminateGuest() {
if (!navigate_src_sent_) if (!navigate_src_sent_ || guest_crashed_)
return; return;
browser_plugin_manager()->Send( browser_plugin_manager()->Send(
new BrowserPluginHostMsg_TerminateGuest(render_view_routing_id_, new BrowserPluginHostMsg_TerminateGuest(render_view_routing_id_,
......
content/renderer/browser_plugin/browser_plugin.h
View file @ 221c4beb
...@@ -89,6 +89,8 @@ class CONTENT_EXPORT BrowserPlugin : ...@@ -89,6 +89,8 @@ class CONTENT_EXPORT BrowserPlugin :
int guest_process_id() const { return guest_process_id_; } int guest_process_id() const { return guest_process_id_; }
// Returns Chrome's route ID for the current guest. // Returns Chrome's route ID for the current guest.
int guest_route_id() const { return guest_route_id_; } int guest_route_id() const { return guest_route_id_; }
// Returns whether the guest process has crashed.
bool guest_crashed() const { return guest_crashed_; }
// Query whether the guest can navigate back to the previous entry. // Query whether the guest can navigate back to the previous entry.
bool CanGoBack() const; bool CanGoBack() const;
......
content/renderer/browser_plugin/browser_plugin_bindings.cc
View file @ 221c4beb
...@@ -709,7 +709,18 @@ class BrowserPluginPropertyBindingSrc : public BrowserPluginPropertyBinding { ...@@ -709,7 +709,18 @@ class BrowserPluginPropertyBindingSrc : public BrowserPluginPropertyBinding {
const NPVariant* variant) OVERRIDE { const NPVariant* variant) OVERRIDE {
std::string new_value = StringFromNPVariant(*variant); std::string new_value = StringFromNPVariant(*variant);
std::string old_value = bindings->instance()->GetSrcAttribute(); std::string old_value = bindings->instance()->GetSrcAttribute();
if (old_value != new_value && !new_value.empty()) { bool guest_crashed = bindings->instance()->guest_crashed();
if ((old_value != new_value) || guest_crashed) {
if (new_value.empty()) {
// Remove the DOM attribute to trigger <webview>'s mutation observer
// when it is restored to its original value again.
bindings->instance()->RemoveDOMAttribute(name());
new_value = old_value;
}
// If the new value was empty then we're effectively resetting the
// attribute to the old value here. This will be picked up by <webview>'s
// mutation observer and will restore the src attribute after it has been
// removed.
UpdateDOMAttribute(bindings, new_value); UpdateDOMAttribute(bindings, new_value);
std::string error_message; std::string error_message;
if (!bindings->instance()->ParseSrcAttribute(&error_message)) { if (!bindings->instance()->ParseSrcAttribute(&error_message)) {
...@@ -724,7 +735,11 @@ class BrowserPluginPropertyBindingSrc : public BrowserPluginPropertyBinding { ...@@ -724,7 +735,11 @@ class BrowserPluginPropertyBindingSrc : public BrowserPluginPropertyBinding {
} }
virtual void RemoveProperty(BrowserPluginBindings* bindings, virtual void RemoveProperty(BrowserPluginBindings* bindings,
NPObject* np_obj) OVERRIDE { NPObject* np_obj) OVERRIDE {
std::string old_value = bindings->instance()->GetSrcAttribute();
// Remove the DOM attribute to trigger the mutation observer when it is
// restored to its original value again.
bindings->instance()->RemoveDOMAttribute(name()); bindings->instance()->RemoveDOMAttribute(name());
UpdateDOMAttribute(bindings, old_value);
} }
private: private:
DISALLOW_COPY_AND_ASSIGN(BrowserPluginPropertyBindingSrc); DISALLOW_COPY_AND_ASSIGN(BrowserPluginPropertyBindingSrc);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment