Cleanup power save blockers after a RenderView crashes.

Cleanup power save blockers after a RenderView crashes.

Looks like RenderFrameDeleted() wasn't called if a RenderView
crashes, which results in the WebContents having some lingering
power save blockers and media player entries.

BUG=455943
TEST=new unittest. DCHECK() no longer fires.

Committed: https://crrev.com/8fc7b4dd54bd2779ce339abc9e0829e98c04c347
Cr-Commit-Position: refs/heads/master@{#314934}

[DaleCurtis, 2015-02-03 21:56:15]

dalecurtis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2015-02-03 22:12:29]

creis@chromium.org changed reviewers:
+ nasko@chromium.org

[Charlie Reis, 2015-02-03 22:12:29]

Hmm, it sounds like we should fix the fact that RenderFrameDeleted isn't being
called.  Nasko is trying to fix issues like that in https://crbug.com/450799.

We can probably use this to fix it in the meantime, but let's put a TODO to
remove it once RenderFrameDeleted is fixed.

Since this appears to be a regression from
https://codereview.chromium.org/264773002/, can you file a bug and add a test
for it?  That will make sure we get it right once RenderFrameDeleted is updated.
 Thanks!

[DaleCurtis, 2015-02-03 23:13:55]

On 2015/02/03 22:12:29, Charlie Reis wrote:
> Hmm, it sounds like we should fix the fact that RenderFrameDeleted isn't being
> called.  Nasko is trying to fix issues like that in https://crbug.com/450799.
> 
> We can probably use this to fix it in the meantime, but let's put a TODO to
> remove it once RenderFrameDeleted is fixed.

Sure, sounds good. I didn't know if it should be called in this case, just that
it wasn't. Let me look around and figure out how suited the web contents unit
test is for such a test. I'll update here once I get it.

> 
> Since this appears to be a regression from
> https://codereview.chromium.org/264773002/, can you file a bug and add a test
> for it?  That will make sure we get it right once RenderFrameDeleted is
updated.
>  Thanks!

The linked CL was never submitted, am I looking at the right CL?

[Charlie Reis, 2015-02-03 23:32:41]

On 2015/02/03 23:13:55, DaleCurtis wrote:
> On 2015/02/03 22:12:29, Charlie Reis wrote:
> > Hmm, it sounds like we should fix the fact that RenderFrameDeleted isn't
being
> > called.  Nasko is trying to fix issues like that in
https://crbug.com/450799.
> > 
> > We can probably use this to fix it in the meantime, but let's put a TODO to
> > remove it once RenderFrameDeleted is fixed.
> 
> Sure, sounds good. I didn't know if it should be called in this case, just
that
> it wasn't. Let me look around and figure out how suited the web contents unit
> test is for such a test. I'll update here once I get it.

Thanks!

> > 
> > Since this appears to be a regression from
> > https://codereview.chromium.org/264773002/, can you file a bug and add a
test
> > for it?  That will make sure we get it right once RenderFrameDeleted is
> updated.
> >  Thanks!
> 
> The linked CL was never submitted, am I looking at the right CL?

It was committed in http://crrev.com/267605, and later in
http://crrev.com/268090.  It's mentioned in the CL description, though I guess
the bots never appended a commit message to the review.

[DaleCurtis, 2015-02-06 00:01:59]

PTAL.

[Charlie Reis, 2015-02-06 00:08:21]

Thanks for the test!

LGTM with nit, and a note to Nasko below.

https://codereview.chromium.org/886603009/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/886603009/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3706: // RenderFrameDeleted().
 See http://crbug.com/455943
nit: End with period.  (Code search gets the link right.)

https://codereview.chromium.org/886603009/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/886603009/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_unittest.cc:3041:
ViewHostMsg_RenderProcessGone(
@nasko FYI: You'll get a conflict from this line in
https://codereview.chromium.org/894843003/.  In your CL, you can drop the
"GetRenderViewHost()" part and switch this to FrameHostMsg.

[DaleCurtis, 2015-02-06 00:10:17]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[DaleCurtis, 2015-02-06 00:10:32]

Thanks for review!

https://codereview.chromium.org/886603009/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/886603009/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3706: // RenderFrameDeleted().
 See http://crbug.com/455943
On 2015/02/06 00:08:21, Charlie Reis wrote:
> nit: End with period.  (Code search gets the link right.)

Done.

[DaleCurtis, 2015-02-06 00:11:04]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-02-06 00:11:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886603009/40001

[nasko, 2015-02-06 00:59:49]

Does the media stack handle this properly, if RenderFrameDeleted is properly
dispatched? I assume I can remove the code in WebContentsImpl once I fix the
other bug, correct?

[commit-bot: I haz the power, 2015-02-06 01:06:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-06 01:07:33]

Patchset 3 (id:??) landed as
https://crrev.com/8fc7b4dd54bd2779ce339abc9e0829e98c04c347
Cr-Commit-Position: refs/heads/master@{#314934}

[DaleCurtis, 2015-02-06 01:39:18]

On 2015/02/06 00:59:49, nasko wrote:
> Does the media stack handle this properly, if RenderFrameDeleted is properly
> dispatched? I assume I can remove the code in WebContentsImpl once I fix the
> other bug, correct?

Yes, the code to clear the blockers is already there:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...