Browser Plugin: More robust recovery from guest crash

Browser Plugin: More robust recovery from guest crash

Guests don't recover from crashes very well. Basically, the only
actions that would properly recover the browser plugin from crash
is the reload method and setting the src attribute. This change 
moves the resetting of the guest_crashed_ flag to UpdateRect in order 
to make crash recovery more robust. If we're receiving new pixels from
guests, then we can assume that the guest has recovered from a crash.

BUG=155324
Test=BrowserPluginHostTest.BackAfterTerminateGuest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161986

[Charlie Reis, 2012-10-12 04:10:25]

Sorry for the delay, and thanks for fixing this.  Mostly looks good, but two
questions below.

https://codereview.chromium.org/11094080/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11094080/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:683:
SimulateMouseClick(test_embedder()->web_contents());
What's the reason for adding all this input event test logic?  If it's just to
check if the guest thinks it is still crashed, would it be easier to just check
something like test_guest()->IsCrashed() here?

https://codereview.chromium.org/11094080/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11094080/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:287: guest_crashed_ = false;
Wouldn't DidNavigate/LoadCommit be a better place for this?  If any page
commits, we know we're no longer crashed.  (That seems closer to the invariant I
would expect, as well as how the browser process handles it, rather than
inferring it from graphical updates.)

[Fady Samuel, 2012-10-12 17:33:06]

https://codereview.chromium.org/11094080/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11094080/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:683:
SimulateMouseClick(test_embedder()->web_contents());
On 2012/10/12 04:10:25, creis wrote:
> What's the reason for adding all this input event test logic?  If it's just to
> check if the guest thinks it is still crashed, would it be easier to just
check
> something like test_guest()->IsCrashed() here?

One of the symptoms manifested by this bug was that input was being dropped on
the floor in BrowserPlugin because it thought it's still crashed [and we
continued to show a sad plugin]. I don't just want to verify that the guest is
no longer crashed, I also want to verify that 1. BrowserPlugin knows that the
guest is longer crashed. 2. BrowserPlugin correctly forwards input to the guest
after it has recovered from crash.

https://codereview.chromium.org/11094080/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11094080/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:287: guest_crashed_ = false;
On 2012/10/12 04:10:25, creis wrote:
> Wouldn't DidNavigate/LoadCommit be a better place for this?  If any page
> commits, we know we're no longer crashed.  (That seems closer to the invariant
I
> would expect, as well as how the browser process handles it, rather than
> inferring it from graphical updates.)

Done.

[Charlie Reis, 2012-10-15 18:17:04]

Thanks, LGTM.

[commit-bot: I haz the power, 2012-10-15 18:17:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11094080/13001

[commit-bot: I haz the power, 2012-10-15 19:38:01]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-10-15 20:16:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11094080/27001

[commit-bot: I haz the power, 2012-10-15 22:51:10]

Change committed as 161986