Don't use process isolation for bookmark apps.

Don't use process isolation for bookmark apps.

BUG=104636
TEST=No process swap on a link to/from a bookmark app.


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

[Charlie Reis, 2011-11-17 02:35:07]

This is just a first draft to see what's involved.  I would still want to add a
bug report and tests before we moved forward.  It works by making both the
browser and renderer think bookmark app URLs are normal URLs, though it may
affect more code than just that.

There may be a better place (either earlier than ExtensionSet or closer to the
process model decisions) to make these checks.

I'm also concerned about the implications this has elsewhere in the code.  For
example, we don't currently prevent bookmark apps from requesting permissions,
so this could cause problems if they ever do.

[Aaron Boodman, 2011-11-17 04:10:24]

What bad thing happens if we just take out the process model changes for all
hosted apps?

http://codereview.chromium.org/8585016/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8585016/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:739: // treated like normal
web pages from the process model's perspective.
Can you just check from_bookmark() here instead of changing ExtensionSet?

[Erik does not do reviews, 2011-11-17 16:40:06]

On 2011/11/17 04:10:24, Aaron Boodman wrote:
> What bad thing happens if we just take out the process model changes for all
> hosted apps?

At a minimum, we'd need to preserve the behavior for the web store and isolated
apps.  It might also lead to us taking longer to track down issues that are
caused by both of those cases (for example, the youtube/search interaction also
affects the web store, but this didn't get escalated until it affected youtube).
 Beyond that, the issues are that I think such a change would just need a fair
amount of air time due to the number of apps affected.

> 
>
http://codereview.chromium.org/8585016/diff/1/chrome/renderer/chrome_content_...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
http://codereview.chromium.org/8585016/diff/1/chrome/renderer/chrome_content_...
> chrome/renderer/chrome_content_renderer_client.cc:739: // treated like normal
> web pages from the process model's perspective.
> Can you just check from_bookmark() here instead of changing ExtensionSet?

[Charlie Reis, 2011-11-17 17:22:10]

On 2011/11/17 16:40:06, Erik Kay wrote:
> On 2011/11/17 04:10:24, Aaron Boodman wrote:
> > What bad thing happens if we just take out the process model changes for all
> > hosted apps?
> 
> At a minimum, we'd need to preserve the behavior for the web store and
isolated
> apps.  It might also lead to us taking longer to track down issues that are
> caused by both of those cases (for example, the youtube/search interaction
also
> affects the web store, but this didn't get escalated until it affected
youtube).
>  Beyond that, the issues are that I think such a change would just need a fair
> amount of air time due to the number of apps affected.
> 

In addition, we grant permissions to hosted app processes.  We'd be granting
those permissions to normal web processes (with security implications) and we
would likely have to change the checks in place for that.

Anyway, I can avoid changing ExtensionSet-- I'll put together a new patch this
morning.

[Charlie Reis, 2011-11-17 20:27:57]

Moved the logic to ChromeContentRendererClient to be closer to the process model
decision.

I've also added a test, but it's disabled until we get a way to load a bookmark
app in a browser test (http://crbug.com/104649).  I chatted with Greg Billock
about it and it seems doable but non-trivial for this CL.  I did verify that the
test passes if I hard-code Extension::from_bookmark() to return true.

http://codereview.chromium.org/8585016/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8585016/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:739: // treated like normal
web pages from the process model's perspective.
On 2011/11/17 04:10:25, Aaron Boodman wrote:
> Can you just check from_bookmark() here instead of changing ExtensionSet?

Done.

[Charlie Reis, 2011-11-17 22:57:32]

Fixed a bug that led to try job failures.  PTAL.

[Aaron Boodman, 2011-11-18 18:18:16]

lgtm

[Aaron Boodman, 2011-11-18 18:19:52]

http://codereview.chromium.org/8585016/diff/3002/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_process_apitest.cc (right):

http://codereview.chromium.org/8585016/diff/3002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_process_apitest.cc:257: // app in browser_tests. 
See http://crbug.com/104649.
These days it should be easier. I believe that these days you can just call
ExtensionService::Add() with any extension object and it will work. So it should
not be too hard to create an extension with the right flags and add it. You can
do that in a separate change if you want.

http://codereview.chromium.org/8585016/diff/3002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8585016/diff/3002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2190: // (excluding bookmark
apps) if the URL came from GetEffectiveURL.
I don't think the last part of this comment is necessary. ExtensionService
should have no knowledge of GetEffectiveURL.

http://codereview.chromium.org/8585016/diff/3002/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8585016/diff/3002/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:741: const Extension*
new_url_extension = extensions->GetByURL(new_url);
Pull this into a helper, so that you don't have to repeat it? Alternatively, you
could add optional flags to GetByURL() and have one like EXCLUDE_BOOKMARK_APPS.

[Charlie Reis, 2011-11-18 19:45:41]

http://codereview.chromium.org/8585016/diff/3002/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_process_apitest.cc (right):

http://codereview.chromium.org/8585016/diff/3002/chrome/browser/extensions/ap...
chrome/browser/extensions/app_process_apitest.cc:257: // app in browser_tests. 
See http://crbug.com/104649.
On 2011/11/18 18:19:52, Aaron Boodman wrote:
> These days it should be easier. I believe that these days you can just call
> ExtensionService::Add() with any extension object and it will work. So it
should
> not be too hard to create an extension with the right flags and add it. You
can
> do that in a separate change if you want.

Ah, that's good to hear.  I'll try for that in a follow-up CL, just to get this
one in sooner.

http://codereview.chromium.org/8585016/diff/3002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8585016/diff/3002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2190: // (excluding bookmark
apps) if the URL came from GetEffectiveURL.
On 2011/11/18 18:19:52, Aaron Boodman wrote:
> I don't think the last part of this comment is necessary. ExtensionService
> should have no knowledge of GetEffectiveURL.

I think it's useful to have that comment, since it explains why we need two
different types of checks for hosted apps below.

http://codereview.chromium.org/8585016/diff/3002/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8585016/diff/3002/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:741: const Extension*
new_url_extension = extensions->GetByURL(new_url);
On 2011/11/18 18:19:52, Aaron Boodman wrote:
> Pull this into a helper, so that you don't have to repeat it? Alternatively,
you
> could add optional flags to GetByURL() and have one like
EXCLUDE_BOOKMARK_APPS.

Done.

[commit-bot: I haz the power, 2011-11-18 19:58:55]

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

[commit-bot: I haz the power, 2011-11-18 20:46:08]

Try job failure for 8585016-11002 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Charlie Reis, 2011-11-18 21:23:11]

Fixed the merge conflict with RenderProcessHost::GetID.  Sending to commit queue
again.

[commit-bot: I haz the power, 2011-11-18 21:23:31]

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

[commit-bot: I haz the power, 2011-11-18 22:40:54]

Change committed as 110772