Ensure extensions and the Chrome Web Store are loaded in new BrowsingInstances.

Ensure extensions and the Chrome Web Store are loaded in new BrowsingInstances.

BUG=174943
TEST=Can't post message to CWS.  See bug for repro steps.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184208

[Charlie Reis, 2013-02-19 19:08:59]

Nasko, here's the CL I mentioned for the postMessage case.  Can you take a look?
 I'll see if I can add a test for it.

[nasko, 2013-02-19 19:20:42]

Just a couple of clarifying questions.

https://codereview.chromium.org/12301013/diff/4001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/12301013/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1026: if (!service)
nit: Put a comment on why it is fine to return early here. Based on my
understanding, this function only deals with extension URLs, so if
ExtensionService is not present, the rest need not be executed.

https://codereview.chromium.org/12301013/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1058:
site_instance->HasProcess() &&
It shouldn't be possible for the site_instance not to have a process at this
point in time, should it?

https://codereview.chromium.org/12301013/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1059:
!service->process_map()->Contains(site_instance->GetProcess()->GetID()))
Why not check with Contains(extension_id, process_id)?

[Charlie Reis, 2013-02-22 05:08:40]

For the test, I'm thinking I want to land it in a separate CL in your new
chrome_security_exploits_browsertest.cc file.  That will avoid having
dependencies or conflicts between our two CLs, making it easier to merge one or
both of them.  Sound ok?

https://codereview.chromium.org/12301013/diff/4001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/12301013/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1026: if (!service)
On 2013/02/19 19:20:42, nasko wrote:
> nit: Put a comment on why it is fine to return early here. Based on my
> understanding, this function only deals with extension URLs, so if
> ExtensionService is not present, the rest need not be executed.

On second thought, I'm going to reduce the behavior delta here.  I was trying to
make the merge with my 123007 fix easier, but there's no reason to force a
change in behavior for the two existing checks below.

I've included a comment but moved this below them for now.

https://codereview.chromium.org/12301013/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1058:
site_instance->HasProcess() &&
On 2013/02/19 19:20:42, nasko wrote:
> It shouldn't be possible for the site_instance not to have a process at this
> point in time, should it?

Actually, I'm not certain about that, so I'd prefer to keep the check.  It's
referring to the RenderProcessHost of the currently visible page, so you could
be typing in a new URL while looking at a sad tab.  In my manual test, it looks
like HasProcess() still returns true after I've killed the tab, but there may be
cases it's gone.

https://codereview.chromium.org/12301013/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1059:
!service->process_map()->Contains(site_instance->GetProcess()->GetID()))
On 2013/02/19 19:20:42, nasko wrote:
> Why not check with Contains(extension_id, process_id)?

Good call.  Done.

[nasko, 2013-02-22 18:03:13]

Adding test later is fine with me.

lgtm

[Charlie Reis, 2013-02-22 18:08:06]

John, can you take a look for chrome/ owner's approval?  I'm basically adding
the current SiteInstance to ShouldSwap so that we can check if it has an
extension process.

[jam, 2013-02-22 20:27:20]

lgtm

[commit-bot: I haz the power, 2013-02-22 20:46:51]

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

[commit-bot: I haz the power, 2013-02-22 23:12:17]

Change committed as 184208