Allow apps with background pages to request process-per-app-instance.

Allow apps with background pages to request process-per-app-instance.
Requires setting background.allow_js_access to false in manifest.

BUG=113444
TEST=Example hosted app has different processes in different tabs.


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

[Charlie Reis, 2012-02-29 02:56:32]

Mihai, can you review?

Drew, you mentioned there was a way to prohibit these apps from opening
background pages manually (comment 24 of the bug).  Is that needed?

https://chromiumcodereview.appspot.com/9508008/diff/1/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9508008/diff/1/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:458:
!extension->allow_background_js_access()) {
Drew or Mihai, is there a way to enforce having no JS access to the background
page?  So far I'm just using it as a signal to the process model.

[Andrew T Wilson (Slow), 2012-02-29 03:20:15]

I've got to run, but I'll try to look at this tomorrow. One note below about how
to actually prevent js access...

https://chromiumcodereview.appspot.com/9508008/diff/1/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9508008/diff/1/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:458:
!extension->allow_background_js_access()) {
On 2012/02/29 02:56:32, creis wrote:
> Drew or Mihai, is there a way to enforce having no JS access to the background
> page?  So far I'm just using it as a signal to the process model.
I think returning false from CanCreateWindow() will do it  - I think that will
prevent requests from the renderer to create a bg window, but will still allow
the browser process to create one. The currently missing piece is how to get
information about the extension "allow_background_js_access()" into the
ExtensionInfoMap so it can be accessed from the I/O thread.

[Charlie Reis, 2012-03-01 00:38:22]

https://chromiumcodereview.appspot.com/9508008/diff/1/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9508008/diff/1/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:458:
!extension->allow_background_js_access()) {
On 2012/02/29 03:20:15, Andrew T Wilson wrote:
> On 2012/02/29 02:56:32, creis wrote:
> > Drew or Mihai, is there a way to enforce having no JS access to the
background
> > page?  So far I'm just using it as a signal to the process model.
> I think returning false from CanCreateWindow() will do it  - I think that will
> prevent requests from the renderer to create a bg window, but will still allow
> the browser process to create one.

Yep, seems to work.

> The currently missing piece is how to get
> information about the extension "allow_background_js_access()" into the
> ExtensionInfoMap so it can be accessed from the I/O thread.

Ugh.  This seems like it should be easy, but it's pretty awkward.  We really
need a better way to attribute a security principal (like an Extension) to any
given request.  Another argument for http://crbug.com/109792.

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/browser/exten...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/browser/exten...
chrome/browser/extensions/app_background_page_apitest.cc:182:
ASSERT_TRUE(RunExtensionTest("app_background_page/no_js")) << message_;
I spent most of the afternoon trying to understand what these
AppBackgroundPageApiTests are doing, without success.  It's a maze of redundant
manifest and script files to me.  Drew, perhaps we can talk about it tomorrow?

[Charlie Reis, 2012-03-01 02:45:22]

Test uploaded.  PTAL.

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/browser/exten...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/browser/exten...
chrome/browser/extensions/app_background_page_apitest.cc:182:
ASSERT_TRUE(RunExtensionTest("app_background_page/no_js")) << message_;
On 2012/03/01 00:38:22, creis wrote:
> I spent most of the afternoon trying to understand what these
> AppBackgroundPageApiTests are doing, without success.  It's a maze of
redundant
> manifest and script files to me.  Drew, perhaps we can talk about it tomorrow?

Thanks for the explanation, Drew!  It would have taken me a while to figure out
the test has both an extension and an app.  Looks like the test is working now.

[Mihai Parparita -not on Chrome, 2012-03-01 02:48:11]

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/browser/chrom...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/browser/chrom...
chrome/browser/chrome_content_browser_client.cc:1178: // create the background
page as long as the permission check below
Where does the creation happen?

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/common/extens...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/common/extens...
chrome/common/extensions/extension.cc:1436: bool
Extension::LoadBackgroundAllowJsAccess(
I'm assuming you based this on LoadBackgroundPersistent, but a simpler approach
than Get/IsType/GetAsBoolean is to use HasKey and GetBoolean (both on
DictionaryValue) directly.

[Mihai Parparita -not on Chrome, 2012-03-01 02:50:17]

On Wed, Feb 29, 2012 at 6:48 PM, <mihaip@chromium.org> wrote:

> https://chromiumcodereview.**appspot.com/9508008/diff/8001/**
>
chrome/browser/chrome_content_**browser_client.cc#newcode1178<https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/browser/chrome_content_browser_client.cc#newcode1178>
> chrome/browser/chrome_content_**browser_client.cc:1178: // create the
> background page as long as the permission check below
> Where does the creation happen?
>

Nevermind, I see that the comment in the new patch has been updated to say
"In this case, only the manifest is permitted to create a background
window", which makes more sense to me (the old one made it sound like
window.open would return null, but the background page would still be
created).

Mihai

[Mihai Parparita -not on Chrome, 2012-03-01 02:58:38]

https://chromiumcodereview.appspot.com/9508008/diff/9013/chrome/test/data/ext...
File
chrome/test/data/extensions/api_test/app_background_page/no_js/manifest.json
(right):

https://chromiumcodereview.appspot.com/9508008/diff/9013/chrome/test/data/ext...
chrome/test/data/extensions/api_test/app_background_page/no_js/manifest.json:7:
"page": "test.html"
Nit: You can use "script": "test.js" to remove the need for the test.html stub.

https://chromiumcodereview.appspot.com/9508008/diff/9013/chrome/test/data/ext...
File chrome/test/data/extensions/api_test/app_background_page/no_js/test.js
(right):

https://chromiumcodereview.appspot.com/9508008/diff/9013/chrome/test/data/ext...
chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:6: // - A
background page is opened via the manifest.
Is this actually checked anywhere (that a background page is opened at all)?

https://chromiumcodereview.appspot.com/9508008/diff/9013/chrome/test/data/ext...
chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:34: var
a_url =
Nit: this should be called aUrl.

[Charlie Reis, 2012-03-01 20:12:12]

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/browser/chrom...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/browser/chrom...
chrome/browser/chrome_content_browser_client.cc:1178: // create the background
page as long as the permission check below
On 2012/03/01 02:48:11, Mihai Parparita wrote:
> Where does the creation happen?

Right, as you noted, this comment was wrong and has been updated.

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/common/extens...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/common/extens...
chrome/common/extensions/extension.cc:1436: bool
Extension::LoadBackgroundAllowJsAccess(
On 2012/03/01 02:48:11, Mihai Parparita wrote:
> I'm assuming you based this on LoadBackgroundPersistent, but a simpler
approach
> than Get/IsType/GetAsBoolean is to use HasKey and GetBoolean (both on
> DictionaryValue) directly.

I'm not sure I follow what you want.  I can't call HasKey and GetBoolean on
manifest_, because ManifestFeatureProvider complains.  If I call them on the
DictionaryValue for "background", then I first have to retrieve the "background"
value using manifest_->Get, and I have to rename the key from
"background.allow_js_access" to "allow_js_access" (arguably less clear).  I'm ok
either way, but it's not clear to me that's an improvement.

https://chromiumcodereview.appspot.com/9508008/diff/9013/chrome/test/data/ext...
File
chrome/test/data/extensions/api_test/app_background_page/no_js/manifest.json
(right):

https://chromiumcodereview.appspot.com/9508008/diff/9013/chrome/test/data/ext...
chrome/test/data/extensions/api_test/app_background_page/no_js/manifest.json:7:
"page": "test.html"
On 2012/03/01 02:58:38, Mihai Parparita wrote:
> Nit: You can use "script": "test.js" to remove the need for the test.html
stub.

Done.

https://chromiumcodereview.appspot.com/9508008/diff/9013/chrome/test/data/ext...
File chrome/test/data/extensions/api_test/app_background_page/no_js/test.js
(right):

https://chromiumcodereview.appspot.com/9508008/diff/9013/chrome/test/data/ext...
chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:6: // - A
background page is opened via the manifest.
On 2012/03/01 02:58:38, Mihai Parparita wrote:
> Is this actually checked anywhere (that a background page is opened at all)?

It's checked in the browser_test before launching this extension.  I've added a
comment to mention that.

I didn't see a way to test it here, at least when looking at the
OpenTwoPagesWithManifest test.  If there is a way, I can add it.

https://chromiumcodereview.appspot.com/9508008/diff/9013/chrome/test/data/ext...
chrome/test/data/extensions/api_test/app_background_page/no_js/test.js:34: var
a_url =
On 2012/03/01 02:58:38, Mihai Parparita wrote:
> Nit: this should be called aUrl.

Done.

[Mihai Parparita -not on Chrome, 2012-03-02 01:10:05]

LGTM

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/common/extens...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/9508008/diff/8001/chrome/common/extens...
chrome/common/extensions/extension.cc:1436: bool
Extension::LoadBackgroundAllowJsAccess(
On 2012/03/01 20:12:12, creis wrote:
> On 2012/03/01 02:48:11, Mihai Parparita wrote:
> > I'm assuming you based this on LoadBackgroundPersistent, but a simpler
> approach
> > than Get/IsType/GetAsBoolean is to use HasKey and GetBoolean (both on
> > DictionaryValue) directly.
> 
> I'm not sure I follow what you want.  I can't call HasKey and GetBoolean on
> manifest_, because ManifestFeatureProvider complains.  If I call them on the
> DictionaryValue for "background", then I first have to retrieve the
"background"
> value using manifest_->Get, and I have to rename the key from
> "background.allow_js_access" to "allow_js_access" (arguably less clear).  I'm
ok
> either way, but it's not clear to me that's an improvement.

Ah, hadn't realized that ManifestFeatureProvider doesn't allow the direct
HasKey/GetBoolean check. Seems fine as is then.

[Andrew T Wilson (Slow), 2012-03-02 05:54:12]

LGTM. Thanks for tackling this!

[commit-bot: I haz the power, 2012-03-02 16:29:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/9508008/17001

[commit-bot: I haz the power, 2012-03-02 18:06:55]

Change committed as 124684