Now closes existing background contents if app opens a new one.

Now closes existing background contents if app opens a new one.

Work around race conditions on startup where an app page creates a background
page while the BackgroundContentsService is also opening the page. We now close
the existing page instead of opening one of the pages up in a visible tab.

Also added a new browser test to verify this functionality.

BUG=98046
TEST=Added new browser_test: AppBackgroundPageApiTest.OpenTwoBackgroundPages.


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

[Andrew T Wilson (Slow), 2011-10-04 01:30:03]

Please take a look.

[brettw, 2011-10-04 05:32:43]

Am I the best reviewer for this? I don't even remember what background pages
are...

[Andrew T Wilson (Slow), 2011-10-04 17:23:24]

I've added Rafael who has more insight into background contents and is probably
a better reviewer.

Brett, you're mainly on here since you and Avi are the only ones in the
browser/tab_contents OWNERS file, so I wanted to give one of you a chance to
comment if you want.

Rafael: PTAL

[rafaelw, 2011-10-04 20:10:28]

I did an overall review and the code mostly looks fine.

I'm concerned about the basic solution to managing the race of just shutting
down any open bg page. What if a TC pages has somehow gotten a handle to it?
Won't it be surprised to find that the page it's holding is now defunct?

I'm sure it's not easy, but it would seem preferable to prevent the race
condition. Have you looked at what that would take?

http://codereview.chromium.org/8116015/diff/4001/chrome/browser/background/ba...
File chrome/browser/background/background_contents_service.cc (right):

http://codereview.chromium.org/8116015/diff/4001/chrome/browser/background/ba...
chrome/browser/background/background_contents_service.cc:362: continue;
It seems wrong to leave the NOTREACHED() here. Also, if reaching this point is
expectable, the comment should explain how it happens.

[Andrew T Wilson (Slow), 2011-10-04 21:08:13]

On 2011/10/04 20:10:28, rafaelw wrote:
> I did an overall review and the code mostly looks fine.
> 
> I'm concerned about the basic solution to managing the race of just shutting
> down any open bg page. What if a TC pages has somehow gotten a handle to it?
> Won't it be surprised to find that the page it's holding is now defunct?

This behavior has been a feature request for a while (the current bheavior makes
it hard for developers to move their background pages to a new URL otherwise).
So I think it's worth doing regardless - the fact that it also addresses this
race condition just motivated me to move this up on my todo list.

I'm not particularly concerned about a TC getting a reference to this page,
since the race condition happens when the TC opens its own page before the
browser-initiated page is created.

> 
> I'm sure it's not easy, but it would seem preferable to prevent the race
> condition. Have you looked at what that would take?
> 
Yeah, it's tricky, because we'd need to route the request to create the
background page through webkit (so, rather than creating a BackgroundContents
directly, we essentially call to the renderer to go through the same codepath
that the javascript window.open() call does). Which has it's own race condition
as it's not certain which creation would execute first.

So, anyhow, not impossible, but not an approach I have time to tackle for this
release. I'm happy to keep the bug open to track this issue, though.

>
http://codereview.chromium.org/8116015/diff/4001/chrome/browser/background/ba...
> File chrome/browser/background/background_contents_service.cc (right):
> 
>
http://codereview.chromium.org/8116015/diff/4001/chrome/browser/background/ba...
> chrome/browser/background/background_contents_service.cc:362: continue;
> It seems wrong to leave the NOTREACHED() here. Also, if reaching this point is
> expectable, the comment should explain how it happens.
Sure. In general, it happens if the user has installed an app (like an
unpackaged app) then manually deletes the app so the BackgroundContentsService
doesn't ever get an "uninstall" notification.

I have a NOTREACHED here because this is an exceptional condition and it's more
likely to happen due to a bug. I'll add a comment, but if you think it should be
downgraded to a DLOG let me know.

[Andrew T Wilson (Slow), 2011-10-04 21:12:05]

Please take another look.

http://codereview.chromium.org/8116015/diff/4001/chrome/browser/background/ba...
File chrome/browser/background/background_contents_service.cc (right):

http://codereview.chromium.org/8116015/diff/4001/chrome/browser/background/ba...
chrome/browser/background/background_contents_service.cc:362: continue;
On 2011/10/04 20:10:28, rafaelw wrote:
> It seems wrong to leave the NOTREACHED() here. Also, if reaching this point is
> expectable, the comment should explain how it happens.
Updated the comment. Let me know if you want this changed to a DLOG(ERROR).

[rafaelw, 2011-10-04 21:56:22]

lgtm

[brettw, 2011-10-04 22:03:47]

http://codereview.chromium.org/8116015/diff/9001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_host_delegate_helper.cc (right):

http://codereview.chromium.org/8116015/diff/9001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_host_delegate_helper.cc:128:
DLOG(WARNING) << "Closing existing BackgroundContents for " << opener_url;
I don't quite get this code. Is this an illegal condition?

I'm also surprised that the right way to close a background contents is to reach
inside its RVH and tell it to close before deleting it. Is that step necessary.
Can it be made less fragile?

[Andrew T Wilson (Slow), 2011-10-05 19:45:58]

http://codereview.chromium.org/8116015/diff/9001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_host_delegate_helper.cc (right):

http://codereview.chromium.org/8116015/diff/9001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_host_delegate_helper.cc:128:
DLOG(WARNING) << "Closing existing BackgroundContents for " << opener_url;
On 2011/10/04 22:03:47, brettw wrote:
> I don't quite get this code. Is this an illegal condition?

The warning is because "reopening an existing BackgroundContents" is a fairly
exceptional condition (which is why it took us several months before we figured
out a repro case).

> 
> I'm also surprised that the right way to close a background contents is to
reach
> inside its RVH and tell it to close before deleting it. Is that step
necessary.
> Can it be made less fragile?
Good point. I've removed the call to ClosePage() and verified that deleting the
BackgroundContents by itself properly closes the page.

[brettw, 2011-10-05 20:03:33]

http://codereview.chromium.org/8116015/diff/9001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_host_delegate_helper.cc (right):

http://codereview.chromium.org/8116015/diff/9001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_host_delegate_helper.cc:128:
DLOG(WARNING) << "Closing existing BackgroundContents for " << opener_url;
On 2011/10/05 19:45:58, Andrew T Wilson wrote:
> On 2011/10/04 22:03:47, brettw wrote:
> > I don't quite get this code. Is this an illegal condition?
> 
> The warning is because "reopening an existing BackgroundContents" is a fairly
> exceptional condition (which is why it took us several months before we
figured
> out a repro case).

Let's think about this in terms of somebody random that sees this message in
their output. What should they do? It's a warning so it must be bad. Should they
fix a bug, report this condition to someone? Unless normal people need to do
something, I'd suggest no warning. If it could be useful to people, it should be
rephrased or made into an INFO.

[Andrew T Wilson (Slow), 2011-10-05 20:49:13]

http://codereview.chromium.org/8116015/diff/9001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_host_delegate_helper.cc (right):

http://codereview.chromium.org/8116015/diff/9001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_host_delegate_helper.cc:128:
DLOG(WARNING) << "Closing existing BackgroundContents for " << opener_url;
On 2011/10/05 20:03:34, brettw wrote:
> On 2011/10/05 19:45:58, Andrew T Wilson wrote:
> > On 2011/10/04 22:03:47, brettw wrote:
> > > I don't quite get this code. Is this an illegal condition?
> > 
> > The warning is because "reopening an existing BackgroundContents" is a
fairly
> > exceptional condition (which is why it took us several months before we
> figured
> > out a repro case).
> 
> Let's think about this in terms of somebody random that sees this message in
> their output. What should they do? It's a warning so it must be bad. Should
they
> fix a bug, report this condition to someone? Unless normal people need to do
> something, I'd suggest no warning. If it could be useful to people, it should
be
> rephrased or made into an INFO.

Done.

[brettw, 2011-10-05 22:39:56]

lgtm

[commit-bot: I haz the power, 2011-10-05 23:16:50]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/8116015/18001

[commit-bot: I haz the power, 2011-10-05 23:16:54]

Presubmit check for 8116015-18001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
 
chrome/test/data/extensions/api_test/app_background_page/two_pages/content_script.js

Presubmit checks took 1.2s to calculate.

[commit-bot: I haz the power, 2011-10-06 00:30:52]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/8116015/18002

[commit-bot: I haz the power, 2011-10-06 02:16:14]

Change committed as 104228