Keep normal popups opened from same-origin iframes in an extension process.

Keep normal popups opened from same-origin iframes in an extension process.
This allows the iframe to script the popup, despite being in an extension.
Also use safer logic for detecting the popup case.

BUG=92669
BUG=87417
TEST=Open a popup from a web iframe inside an extension page and script it.


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

[Charlie Reis, 2011-08-17 01:40:11]

Mihai-- can you review the workaround for the popup behavior in
CrossesExtensionExtents, as we've been discussing on issue 92669?  I'm uncertain
which way to go on the AppProcess test behavior.

Adam-- can you review the way I'm using WebSecurityOrigin?  Not entirely sure
I've done that correctly.  Also, the rest of this CL should resolve your concern
from issue 87417.

I'll try to put a regression test for 92669 together in the mean time.  Thanks!

[Mihai Parparita -not on Chrome, 2011-08-17 20:33:09]

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

http://codereview.chromium.org/7624011/diff/1007/chrome/browser/extensions/ap...
chrome/browser/extensions/app_process_apitest.cc:137: // is to require the
opener URL to be a non-extension URL in order to keep the
I think requiring this makes sense (it would still fix bug 92669, right?). That
seems closer to the ideal of having no non-app URLs in the app's process.

[Charlie Reis, 2011-08-17 21:55:24]

I've added a test similar to the one from issue 89272, but I'm having trouble
with it.  When the popup opens, it's apparently added to BrowserList but not as
the LastActiveBrowser, so the assert on line 377 is failing.  That part works if
I remove my fix (and thus introduce a process swap).

Mihai, any ideas what might be happening?  I've tried WaitForNewBrowser, but
that just hangs because the browser has already been created.

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

http://codereview.chromium.org/7624011/diff/1007/chrome/browser/extensions/ap...
chrome/browser/extensions/app_process_apitest.cc:137: // is to require the
opener URL to be a non-extension URL in order to keep the
On 2011/08/17 20:33:09, Mihai Parparita wrote:
> I think requiring this makes sense (it would still fix bug 92669, right?).
That
> seems closer to the ideal of having no non-app URLs in the app's process.

Agreed-- I've updated the logic so we don't affect this case.

[Mihai Parparita -not on Chrome, 2011-08-17 22:16:39]

On 2011/08/17 21:55:24, creis wrote:
> I've added a test similar to the one from issue 89272, but I'm having trouble
> with it.  When the popup opens, it's apparently added to BrowserList but not
as
> the LastActiveBrowser, so the assert on line 377 is failing.  That part works
if
> I remove my fix (and thus introduce a process swap).
> 
> Mihai, any ideas what might be happening?  I've tried WaitForNewBrowser, but
> that just hangs because the browser has already been created.

Not sure why that would happen. What platform is this on? Pawel might have ideas
too.

Mihai

[Charlie Reis, 2011-08-17 23:10:04]

On 2011/08/17 22:16:39, Mihai Parparita wrote:
> On 2011/08/17 21:55:24, creis wrote:
> > I've added a test similar to the one from issue 89272, but I'm having
trouble
> > with it.  When the popup opens, it's apparently added to BrowserList but not
> as
> > the LastActiveBrowser, so the assert on line 377 is failing.  That part
works
> if
> > I remove my fix (and thus introduce a process swap).
> > 
> > Mihai, any ideas what might be happening?  I've tried WaitForNewBrowser, but
> > that just hangs because the browser has already been created.
> 
> Not sure why that would happen. What platform is this on? Pawel might have
ideas
> too.
> 
> Mihai

Bizarre.  It's failing for me on Linux, but the test just passed on the Linux
try job.  I verified that SetLastActive usually gets called during the OpenURL
call of a cross-site transition, and that it isn't being called on the popup
when no cross-site transition is involved.  It puzzles me that the same-site
popups in that test (e.g., many of the WindowOpenHelper calls) are able to work.

I'm hesitant to check it in without having it pass on my local machine, even if
the try job passes.  I'll look into the same-site calls to WindowOpenHelper to
see why they're working.

[Charlie Reis, 2011-08-18 00:59:19]

Problem resolved!  I found a way to make sure the new window was added as the
LastActive, using window()->Show().

Mihai and Adam, can you take another look?  Thanks!

[Mihai Parparita -not on Chrome, 2011-08-18 01:04:50]

LGTM. John will need to approve the content/renderer changes.

[abarth-chromium, 2011-08-18 18:06:52]

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

http://codereview.chromium.org/7624011/diff/5012/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:699:
opener.canAccess(new_origin))
Usually when you want to compare an origin to a URL, you want to use canRequest.

[Charlie Reis, 2011-08-18 18:11:57]

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

http://codereview.chromium.org/7624011/diff/5012/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:699:
opener.canAccess(new_origin))
On 2011/08/18 18:06:52, abarth wrote:
> Usually when you want to compare an origin to a URL, you want to use
canRequest.

That would certainly be easier, since I wouldn't need to create a
WebSecurityOrigin for new_origin.

In this case, though, I chose canAccess because I'm basing the decision on
whether the opener will be able to script the popup.  Is canRequest the right
choice for that?

[abarth-chromium, 2011-08-18 18:18:11]

On Thu, Aug 18, 2011 at 11:11 AM,  <creis@chromium.org> wrote:
>
>
http://codereview.chromium.org/7624011/diff/5012/chrome/renderer/chrome_conte...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
>
>
http://codereview.chromium.org/7624011/diff/5012/chrome/renderer/chrome_conte...
> chrome/renderer/chrome_content_renderer_client.cc:699:
> opener.canAccess(new_origin))
> On 2011/08/18 18:06:52, abarth wrote:
>>
>> Usually when you want to compare an origin to a URL, you want to use
>
> canRequest.
>
> That would certainly be easier, since I wouldn't need to create a
> WebSecurityOrigin for new_origin.
>
> In this case, though, I chose canAccess because I'm basing the decision
> on whether the opener will be able to script the popup.  Is canRequest
> the right choice for that?

There's no way to answer that question definitively.  What you've got
now and canRequest give you different approximations.  canRequest is
more optimistic than canAccess, which is also somewhat optimistic.
The only way to know for sure is to actually load the popup and check,
but of course then it's too late.

Adam

[jam, 2011-08-18 18:41:54]

content lgtm

[Charlie Reis, 2011-08-18 18:44:06]

On 2011/08/18 18:18:11, abarth wrote:
> On Thu, Aug 18, 2011 at 11:11 AM,  <mailto:creis@chromium.org> wrote:
> > In this case, though, I chose canAccess because I'm basing the decision
> > on whether the opener will be able to script the popup. &nbsp;Is canRequest
> > the right choice for that?
> 
> There's no way to answer that question definitively.  What you've got
> now and canRequest give you different approximations.  canRequest is
> more optimistic than canAccess, which is also somewhat optimistic.
> The only way to know for sure is to actually load the popup and check,
> but of course then it's too late.
> 
> Adam

Fair enough-- the URL could redirect, for example, and then the opener wouldn't
be able to script the page.  We're not actually making the final security
decision here, though.  The usual logic will kick in to allow or deny the script
access (in which case it will use canAccess).

Here, we're just making a choice of whether we think it's likely that the opener
will be able to script the popup, in which case we want to keep it in the same
process.  It sounds like we want to be optimistic that it will be able to script
the popup, so I'll switch it to canRequest.

I suppose it's still possible that we'll swap processes and break scripting if a
popup to a cross-origin URL redirects to a page in the same origin as the
iframe, but I don't think we care about that case.  We're not trying to keep all
web URLs in the extension process, just the ones that are likely to script each
other.

I'll ping you when the new CL is up.

[Charlie Reis, 2011-08-18 18:54:31]

Thanks John.  Adam, the new canRequest version is up.

Charlie

[abarth-chromium, 2011-08-18 19:16:12]

This is fine with me.  I don't know this code well enough to give you an
official stamp, but you've got plenty of those already.  :)

[commit-bot: I haz the power, 2011-08-18 23:10:47]

Change committed as 97386