Don't recreate background windows when allow_js_access if false

Don't recreate background windows when allow_js_access if false

Ignore calls to window.open(..., "background") with allow_js_access: false when
there's already a BackgroundContents instance.

BUG=122408
R=creis@chromium.org


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

[Mihai Parparita -not on Chrome, 2012-04-06 21:10:05]

http://codereview.chromium.org/10012053/diff/1/chrome/test/data/extensions/ap...
File chrome/test/data/extensions/api_test/app_background_page/no_js/test.js
(right):

http://codereview.chromium.org/10012053/diff/1/chrome/test/data/extensions/ap...
chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:71: // We
wait for a bit before declaring the test as passed, since
This is kind of lame, but I don't have any other ideas for making this more
testable (except for making MaybeCreateBackgroundContents send a notification
when it ignores the window.open request, but I'm not sure if a test-only
notification is appropriate).

[Charlie Reis, 2012-04-06 21:55:23]

Seems reasonable to me, but I wonder if we do want to notify the page somehow if
we hit this case.

https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/browser/extensi...
chrome/browser/extensions/app_background_page_apitest.cc:150: // window.open
calls recreate instances, instead of being no-ops)
Nice.  I didn't realize we had TestNotificationTracker-- I'll have to start
using it.

nit: End comment with period.

https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/app_background_page/no_js/test.js
(right):

https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/exten...
chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:57:
chrome.test.notifyFail('Background page loaded more than once.');
nit: indent

https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/exten...
chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:71: // We
wait for a bit before declaring the test as passed, since
On 2012/04/06 21:10:05, Mihai Parparita wrote:
> This is kind of lame, but I don't have any other ideas for making this more
> testable (except for making MaybeCreateBackgroundContents send a notification
> when it ignores the window.open request, but I'm not sure if a test-only
> notification is appropriate).

Yeah, this is unfortunate.  If I understand correctly, we're waiting 2 seconds
to make sure the test doesn't fail.  That won't be flaky if it passes, but it
adds to test latency and would be flaky if it started failing due to a
regression.

I'm also hesitant to add a notification, but it's worth noting that web
developers are going to see the same thing.  If they're not sure if their
background page exists (or if it has gotten stuck), won't they be in the same
boat?  (Sending a request, and then nothing happens.)

[Mihai Parparita -not on Chrome, 2012-04-06 22:18:25]

+Peter and David, who are the representative developers in this case.

Do you guys have any preferences for how (if at all) you'd like to be
notified of window.open failing because a background contents already
exists? IIRC your plan is to postMessage to the shared worker started by
the background page, and to use that to determine if the background page
already exists. Do you need any other indicator?

Mihai

On Fri, Apr 6, 2012 at 2:55 PM, <creis@chromium.org> wrote:

> Seems reasonable to me, but I wonder if we do want to notify the page
> somehow if
> we hit this case.
>
>
> https://chromiumcodereview.**appspot.com/10012053/diff/1/**
>
chrome/browser/extensions/app_**background_page_apitest.cc<https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/browser/extensions/app_background_page_apitest.cc>
> File chrome/browser/extensions/app_**background_page_apitest.cc (right):
>
> https://chromiumcodereview.**appspot.com/10012053/diff/1/**
>
chrome/browser/extensions/app_**background_page_apitest.cc#**newcode150<https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/browser/extensions/app_background_page_apitest.cc#newcode150>
> chrome/browser/extensions/app_**background_page_apitest.cc:**150: //
>
> window.open calls recreate instances, instead of being no-ops)
> Nice.  I didn't realize we had TestNotificationTracker-- I'll have to
> start using it.
>
> nit: End comment with period.
>
> https://chromiumcodereview.**appspot.com/10012053/diff/1/**
>
chrome/test/data/extensions/**api_test/app_background_page/**no_js/test.js<https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/extensions/api_test/app_background_page/no_js/test.js>
> File
> chrome/test/data/extensions/**api_test/app_background_page/**no_js/test.js
> (right):
>
> https://chromiumcodereview.**appspot.com/10012053/diff/1/**
> chrome/test/data/extensions/**api_test/app_background_page/**
>
no_js/test.js#newcode57<https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/extensions/api_test/app_background_page/no_js/test.js#newcode57>
> chrome/test/data/extensions/**api_test/app_background_page/**
> no_js/test.js:57:
>
> chrome.test.notifyFail('**Background page loaded more than once.');
> nit: indent
>
> https://chromiumcodereview.**appspot.com/10012053/diff/1/**
> chrome/test/data/extensions/**api_test/app_background_page/**
>
no_js/test.js#newcode71<https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/extensions/api_test/app_background_page/no_js/test.js#newcode71>
>
> chrome/test/data/extensions/**api_test/app_background_page/**
> no_js/test.js:71:
> // We wait for a bit before declaring the test as passed, since
> On 2012/04/06 21:10:05, Mihai Parparita wrote:
>
>> This is kind of lame, but I don't have any other ideas for making this
>>
> more
>
>> testable (except for making MaybeCreateBackgroundContents send a
>>
> notification
>
>> when it ignores the window.open request, but I'm not sure if a
>>
> test-only
>
>> notification is appropriate).
>>
>
> Yeah, this is unfortunate.  If I understand correctly, we're waiting 2
> seconds to make sure the test doesn't fail.  That won't be flaky if it
> passes, but it adds to test latency and would be flaky if it started
> failing due to a regression.
>
> I'm also hesitant to add a notification, but it's worth noting that web
> developers are going to see the same thing.  If they're not sure if
> their background page exists (or if it has gotten stuck), won't they be
> in the same boat?  (Sending a request, and then nothing happens.)
>
>
https://chromiumcodereview.**appspot.com/10012053/<https://chromiumcodereview...
>

[Mihai Parparita -not on Chrome, 2012-04-06 22:22:09]

https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/browser/extensi...
chrome/browser/extensions/app_background_page_apitest.cc:150: // window.open
calls recreate instances, instead of being no-ops)
On 2012/04/06 21:55:24, creis wrote:
> nit: End comment with period.

Fixed.

https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/app_background_page/no_js/test.js
(right):

https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/exten...
chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:57:
chrome.test.notifyFail('Background page loaded more than once.');
On 2012/04/06 21:55:24, creis wrote:
> nit: indent

Fixed.

[psolderitsch_google.com, 2012-04-06 22:36:57]

On Fri, Apr 6, 2012 at 6:18 PM, Mihai Parparita <mihaip@chromium.org> wrote:
> +Peter and David, who are the representative developers in this case.
>
> Do you guys have any preferences for how (if at all) you'd like to be
> notified of window.open failing because a background contents already
> exists? IIRC your plan is to postMessage to the shared worker started by the
> background page, and to use that to determine if the background page already
> exists. Do you need any other indicator?

I'm not very concerned about window.open being a no-op if it already
exists, because I don't think there's any meaningful action we could
take if it was open already anyway. Dave may have other thoughts
though.

Our flow on startup will be something very simple, like:

if (this is chrome 19 or higher) {
   window.open(our background page) // ensures it's running
}

Beyond ensuring that the page has been loaded, we expect that the
page's lifecycle will be mostly managed by the page itself (i.e.,
deciding when to reload or if it ever needs to close).

What I think would be nice to have is a way to actually *close* the
background window from a foreground window, but I understand why
that's difficult or impossible. I'm just thinking of wanting a
failsafe if we ever need to ensure that the background window was
closed (or an easy way to tell Chrome to use a new url for all future
automatic loads of the background page on Chrome restart). As
discussed previously, the workaround is to use the shared worker to
have the background page call close() on itself, which while unwieldy
should work fine. We don't want to create even more work on your side.

Dave - thoughts?

-Peter

>
> Mihai
>
> On Fri, Apr 6, 2012 at 2:55 PM, <creis@chromium.org> wrote:
>>
>> Seems reasonable to me, but I wonder if we do want to notify the page
>> somehow if
>> we hit this case.
>>
>>
>>
>>
https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/browser/extensi...
>> File chrome/browser/extensions/app_background_page_apitest.cc (right):
>>
>>
>>
https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/browser/extensi...
>> chrome/browser/extensions/app_background_page_apitest.cc:150: //
>>
>> window.open calls recreate instances, instead of being no-ops)
>> Nice.  I didn't realize we had TestNotificationTracker-- I'll have to
>> start using it.
>>
>> nit: End comment with period.
>>
>>
>>
https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/exten...
>> File
>> chrome/test/data/extensions/api_test/app_background_page/no_js/test.js
>> (right):
>>
>>
>>
https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/exten...
>> chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:57:
>>
>> chrome.test.notifyFail('Background page loaded more than once.');
>> nit: indent
>>
>>
>>
https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/exten...
>>
>> chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:71:
>> // We wait for a bit before declaring the test as passed, since
>> On 2012/04/06 21:10:05, Mihai Parparita wrote:
>>>
>>> This is kind of lame, but I don't have any other ideas for making this
>>
>> more
>>>
>>> testable (except for making MaybeCreateBackgroundContents send a
>>
>> notification
>>>
>>> when it ignores the window.open request, but I'm not sure if a
>>
>> test-only
>>>
>>> notification is appropriate).
>>
>>
>> Yeah, this is unfortunate.  If I understand correctly, we're waiting 2
>> seconds to make sure the test doesn't fail.  That won't be flaky if it
>> passes, but it adds to test latency and would be flaky if it started
>> failing due to a regression.
>>
>> I'm also hesitant to add a notification, but it's worth noting that web
>> developers are going to see the same thing.  If they're not sure if
>> their background page exists (or if it has gotten stuck), won't they be
>> in the same boat?  (Sending a request, and then nothing happens.)
>>
>> https://chromiumcodereview.appspot.com/10012053/
>
>

[dbk_google.com, 2012-04-07 00:59:31]

I have nothing to add, I think Peter's proposed approach is fine.

-Dave

On Fri, Apr 6, 2012 at 5:36 PM, Peter Solderitsch
<psolderitsch@google.com>wrote:

> On Fri, Apr 6, 2012 at 6:18 PM, Mihai Parparita <mihaip@chromium.org>
> wrote:
> > +Peter and David, who are the representative developers in this case.
> >
> > Do you guys have any preferences for how (if at all) you'd like to be
> > notified of window.open failing because a background contents already
> > exists? IIRC your plan is to postMessage to the shared worker started by
> the
> > background page, and to use that to determine if the background page
> already
> > exists. Do you need any other indicator?
>
> I'm not very concerned about window.open being a no-op if it already
> exists, because I don't think there's any meaningful action we could
> take if it was open already anyway. Dave may have other thoughts
> though.
>
> Our flow on startup will be something very simple, like:
>
> if (this is chrome 19 or higher) {
>   window.open(our background page) // ensures it's running
> }
>
> Beyond ensuring that the page has been loaded, we expect that the
> page's lifecycle will be mostly managed by the page itself (i.e.,
> deciding when to reload or if it ever needs to close).
>
> What I think would be nice to have is a way to actually *close* the
> background window from a foreground window, but I understand why
> that's difficult or impossible. I'm just thinking of wanting a
> failsafe if we ever need to ensure that the background window was
> closed (or an easy way to tell Chrome to use a new url for all future
> automatic loads of the background page on Chrome restart). As
> discussed previously, the workaround is to use the shared worker to
> have the background page call close() on itself, which while unwieldy
> should work fine. We don't want to create even more work on your side.
>
> Dave - thoughts?
>
> -Peter
>
> >
> > Mihai
> >
> > On Fri, Apr 6, 2012 at 2:55 PM, <creis@chromium.org> wrote:
> >>
> >> Seems reasonable to me, but I wonder if we do want to notify the page
> >> somehow if
> >> we hit this case.
> >>
> >>
> >>
> >>
>
https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/browser/extensi...
> >> File chrome/browser/extensions/app_background_page_apitest.cc (right):
> >>
> >>
> >>
>
https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/browser/extensi...
> >> chrome/browser/extensions/app_background_page_apitest.cc:150: //
> >>
> >> window.open calls recreate instances, instead of being no-ops)
> >> Nice.  I didn't realize we had TestNotificationTracker-- I'll have to
> >> start using it.
> >>
> >> nit: End comment with period.
> >>
> >>
> >>
>
https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/exten...
> >> File
> >> chrome/test/data/extensions/api_test/app_background_page/no_js/test.js
> >> (right):
> >>
> >>
> >>
>
https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/exten...
> >>
> chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:57:
> >>
> >> chrome.test.notifyFail('Background page loaded more than once.');
> >> nit: indent
> >>
> >>
> >>
>
https://chromiumcodereview.appspot.com/10012053/diff/1/chrome/test/data/exten...
> >>
> >>
> chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:71:
> >> // We wait for a bit before declaring the test as passed, since
> >> On 2012/04/06 21:10:05, Mihai Parparita wrote:
> >>>
> >>> This is kind of lame, but I don't have any other ideas for making this
> >>
> >> more
> >>>
> >>> testable (except for making MaybeCreateBackgroundContents send a
> >>
> >> notification
> >>>
> >>> when it ignores the window.open request, but I'm not sure if a
> >>
> >> test-only
> >>>
> >>> notification is appropriate).
> >>
> >>
> >> Yeah, this is unfortunate.  If I understand correctly, we're waiting 2
> >> seconds to make sure the test doesn't fail.  That won't be flaky if it
> >> passes, but it adds to test latency and would be flaky if it started
> >> failing due to a regression.
> >>
> >> I'm also hesitant to add a notification, but it's worth noting that web
> >> developers are going to see the same thing.  If they're not sure if
> >> their background page exists (or if it has gotten stuck), won't they be
> >> in the same boat?  (Sending a request, and then nothing happens.)
> >>
> >> https://chromiumcodereview.appspot.com/10012053/
> >
> >
>



-- 
-Dave

[Charlie Reis, 2012-04-07 01:06:03]

Ok.  If we don't need the notification, the overhead of adding it for the test
seems unnecessary.  I'm not thrilled with the test's 2 second delay, but I don't
expect it to be flaky if the feature's working.  LGTM.

[Mihai Parparita -not on Chrome, 2012-04-07 01:37:47]

+Scott for browser/ui OWNERS

[Andrew T Wilson (Slow), 2012-04-07 02:45:25]

drive-by: LGTM with one nit

https://chromiumcodereview.appspot.com/10012053/diff/2002/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/app_background_page/no_js/test.js
(right):

https://chromiumcodereview.appspot.com/10012053/diff/2002/chrome/test/data/ex...
chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:69: {url:
launchUrl },
no space after launchUrl

[Mihai Parparita -not on Chrome, 2012-04-09 00:40:15]

I'm unexpectedly going on paternity leave two weeks earlier than expected, so I
won't be able to land this CL. Given that Docs needs this change for their
offline support, I think it should be checked in sooner rather than later.

Charlie and/or Andrew, can you take this over (once Scott LGTMs), and if bakes
on dev/canary, merge it into the M19 branch?

Thanks,
Mihai

[Andrew T Wilson (Slow), 2012-04-09 02:04:44]

No worries, we'll get it landed. Thanks for handing it off, and congratulations!

[sky, 2012-04-09 15:16:19]

LGTM

[commit-bot: I haz the power, 2012-04-09 15:18:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/10012053/2002

[commit-bot: I haz the power, 2012-04-09 16:44:15]

Change committed as 131370