Try to restore window.opener when opening blocked popups

Try to restore window.opener when opening blocked popups

When we block a popup, and later open it again, we try to set the opener. It works if the popup ends up being hosted in the same process.

Also added a test that session storage is not cloned.

BUG=38458
R=ajwong@chromium.org, bauerb@chromium.org, creis@chromium.org
TEST=BetterPopupBlockerTest.*

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

[jochen (gone - plz use gerrit), 2013-07-27 21:17:48]

plz review

John, //content
Bernhard, everything

[jochen (gone - plz use gerrit), 2013-07-29 16:03:07]

I've updated the CL to include handling of the opener.

John, can you please review the content/public change after Charlie approves
Charlie, can you please review the content/browser bits. I guess you want me to
write a unit test for RVHM. I'll add that soon.
Bernhard, please review the overall CL

[jochen (gone - plz use gerrit), 2013-07-29 19:49:06]

I discovered that I also don't have a way to force the site instance for a
navigation, which I need to do to have the popup exist in the same process as
its opener...

[Charlie Reis, 2013-07-30 01:26:29]

Thanks for taking the session storage namespace into account and finding the
corner case with opener RVHs.  There's some issues I came across below, though.

https://codereview.chromium.org/20924002/diff/17001/chrome/browser/ui/blocked...
File chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc (right):

https://codereview.chromium.org/20924002/diff/17001/chrome/browser/ui/blocked...
chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc:234: void
RunCheckTest(const base::FilePath& test_name, bool expect_new_browser) {
Can you add a comment here saying something about how each test is expected to
behave (e.g., get a popup blocked, then allow it to open, possibly in a new
window)?  Due to the "if the popup blocker blocked" comment below, I was
confused about whether all tests expect the popup to be blocked.

https://codereview.chromium.org/20924002/diff/17001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/20924002/diff/17001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:322: // If we want to set the opener, we
need to use the same SiteInstance.
I'm skeptical about this, since it shouldn't always be the same SiteInstance. 
We have careful logic in WebContentsImpl::CreateNewWindow about which
SiteInstance to use, and this approach loses that.  We probably need to find a
better way to handle this.

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.cc:152: // Ensure that we
have created RVHs for the new RVH's opener chain if
I think this makes sense, but I would be very surprised if your code depends on
it.  It should only matter if the current tab has crashed and we're doing a
reload of the same SiteInstance.

Perhaps we should move it to a separate CL from the rest of this.

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:294: bool propagate_opener) {
Why switch from suppress_opener, which is used commonly elsewhere?

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:303: // It's not possible to
use a guest as opener.
That's not true.  Fady changed WebContentsImpl::CreateNewWindow to support
window.open from guests.

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:306: // Clone the session
storage namespace of the opener.
This code only copies the session storage namespace for the current
SiteInstance, which will be a problem if the popup navigates to a different
SiteInstance that had previously been open in the opener window.

The logic in CreateWithSessionStorage already handles this by copying over the
whole map.  Is there some way that we can make these various functions share
code?  It's getting a little unwieldy with Create, CreateWithSessionStorage, and
two CreateWithOpeners, though I'm not sure if they're easy to combine.  (They
all take in CreateParams, so it seems like it might be possible.)

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:316:
->GetController().GetSessionStorageNamespaceMap().find(partition_id);
nit: Please be consistent about -> being at the end or beginning of a line (vs
line 308).  Personally, I prefer -> at the end (like line 308) and . at the
beginning (like line 323).

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:452: WebContentsImpl*
WebContentsImpl::CreateWithOpener(
I'm finding it confusing that we have two different CreateWithOpener functions.

https://codereview.chromium.org/20924002/diff/17001/content/public/browser/we...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/20924002/diff/17001/content/public/browser/we...
content/public/browser/web_contents.h:103: // You do not want to call this. If
you think you do, make sure you completely
Albert's the one to confirm with if we end up calling or re-using this method. 
The paragraph above does say that we should be using it for window.open.

https://codereview.chromium.org/20924002/diff/17001/content/public/browser/we...
content/public/browser/web_contents.h:112: // |opener| if it is non-NULL. If
|propage_opener| is true, |opener| is also
nit: typo in propagate_opener

[Bernhard Bauer, 2013-07-30 08:34:50]

https://codereview.chromium.org/20924002/diff/17001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/20924002/diff/17001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:347: DCHECK(!params.should_set_opener ||
Wait, you have here

  if (a || b) {
    DCHECK(!a || b);
  }

Is that really the behavior you want?

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:301:
CHECK(params.site_instance);
I assume you have good reasons for using a CHECK over a DCHECK here? If it's
just "this shouldn't happen", I'd prefer a DCHECK for that.

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:308:
GetContentClient()->browser()->
Nit: This needs to be indented four spaces more.

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:318:
opener->GetController().GetSessionStorageNamespaceMap().end());
If the partition ID isn't found, you'll crash later anyway.

[jochen (gone - plz use gerrit), 2013-07-30 14:50:39]

https://codereview.chromium.org/20924002/diff/17001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/20924002/diff/17001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:322: // If we want to set the opener, we
need to use the same SiteInstance.
On 2013/07/30 01:26:30, creis wrote:
> I'm skeptical about this, since it shouldn't always be the same SiteInstance. 
> We have careful logic in WebContentsImpl::CreateNewWindow about which
> SiteInstance to use, and this approach loses that.  We probably need to find a
> better way to handle this.

The popup needs to be in the same process as the opener, otherwise it can't
access window.opener.

In fact, setting the site_instance here is a no-op:
NavigationController::LoadURL will create a NavigationEntry with a NULL site
instance anyway, and so
RenderViewHostController::PrepareRenderViewHostForNavigation creates a new one
if the URL differs from the site url.

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.cc:152: // Ensure that we
have created RVHs for the new RVH's opener chain if
On 2013/07/30 01:26:30, creis wrote:
> I think this makes sense, but I would be very surprised if your code depends
on
> it.  It should only matter if the current tab has crashed and we're doing a
> reload of the same SiteInstance.
> 
> Perhaps we should move it to a separate CL from the rest of this.

done.

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:306: // Clone the session
storage namespace of the opener.
On 2013/07/30 01:26:30, creis wrote:
> This code only copies the session storage namespace for the current
> SiteInstance, which will be a problem if the popup navigates to a different
> SiteInstance that had previously been open in the opener window.

No. it clones it (see line 320).

> 
> The logic in CreateWithSessionStorage already handles this by copying over the
> whole map.  Is there some way that we can make these various functions share
> code?  It's getting a little unwieldy with Create, CreateWithSessionStorage,
and
> two CreateWithOpeners, though I'm not sure if they're easy to combine.  (They
> all take in CreateParams, so it seems like it might be possible.)

John suggested to move the cloning logic to browser navigator and use
CreateWithSessionStorage.

[jochen (gone - plz use gerrit), 2013-07-30 15:56:55]

Addressed all other comments.

PTAL

This still doesn't work if the site and the popup would go into different site
instance.

Charlie, I consider adding a bool to LoadURLParams that says: use the
SiteInstance of the WebContents. wdyt?

Note that WebContents::CreateNewContents does the same (except that it doesn't
go through LoadURLWithParams but the renderer navigates the popup).

[jochen (gone - plz use gerrit), 2013-07-30 19:25:59]

also added the LoadURLParams change.

PTAL

[Bernhard Bauer, 2013-07-31 08:01:04]

lgtm

[jam, 2013-07-31 15:10:37]

are you waiting for me? i defer to creis, looks fine to me

[Charlie Reis, 2013-07-31 17:16:16]

This change doesn't seem right to me.  We seem to have different understandings
of how popups should behave here, so perhaps it's worthwhile to find a time to
meet and discuss.

https://codereview.chromium.org/20924002/diff/17001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/20924002/diff/17001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:322: // If we want to set the opener, we
need to use the same SiteInstance.
On 2013/07/30 14:50:39, jochen wrote:
> On 2013/07/30 01:26:30, creis wrote:
> > I'm skeptical about this, since it shouldn't always be the same
SiteInstance. 
> > We have careful logic in WebContentsImpl::CreateNewWindow about which
> > SiteInstance to use, and this approach loses that.  We probably need to find
a
> > better way to handle this.
> 
> The popup needs to be in the same process as the opener, otherwise it can't
> access window.opener.

That's not true.  Creating opener RVHs allows it to access a placeholder
window.opener when it's cross-site.

> 
> In fact, setting the site_instance here is a no-op:
> NavigationController::LoadURL will create a NavigationEntry with a NULL site
> instance anyway, and so
> RenderViewHostController::PrepareRenderViewHostForNavigation creates a new one
> if the URL differs from the site url.

What is RenderViewHostController::PrepareRenderViewHostForNavigation?  Do you
mean RenderViewHostManager::UpdateRendererStateForNavigate?  If we're going
through that, we'll have the opposite problem-- creating a new SiteInstance more
frequently than we should.  Most cross-site window.open calls should stay in the
same SiteInstance-- only special cases like rel=noreferrer or the new
site-per-process logic should swap.

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/20924002/diff/17001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:306: // Clone the session
storage namespace of the opener.
On 2013/07/30 14:50:39, jochen wrote:
> On 2013/07/30 01:26:30, creis wrote:
> > This code only copies the session storage namespace for the current
> > SiteInstance, which will be a problem if the popup navigates to a different
> > SiteInstance that had previously been open in the opener window.
> 
> No. it clones it (see line 320).

You're only cloning a single SessionStorageNamespace below.  Each WebContents
has an entire map, and the whole map needs to be cloned, as in
CreateWithSessionStorage.

> 
> > 
> > The logic in CreateWithSessionStorage already handles this by copying over
the
> > whole map.  Is there some way that we can make these various functions share
> > code?  It's getting a little unwieldy with Create, CreateWithSessionStorage,
> and
> > two CreateWithOpeners, though I'm not sure if they're easy to combine. 
(They
> > all take in CreateParams, so it seems like it might be possible.)
> 
> John suggested to move the cloning logic to browser navigator and use
> CreateWithSessionStorage.

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:691:
->GetStoragePartitionIdForSite(site);
nit: Arrow on previous line to be consistent (e.g., with
SessionStorageNamespace::Clone).

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/profiles/p...
chrome/browser/profiles/profile.h:386: std::string
GetStoragePartitionIdForSite(const GURL& site);
I think Albert will need to review the storage partition / session storage
namespace changes.

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:260:
load_url_params.should_stay_in_site_instance =
This is incorrect.  Setting the opener will not always imply that you should
stay in the same SiteInstance.  More comments below.

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:331: // In order for window.opener to be
accessible, the target contents needs to
This reasoning is false.  The reason that we create opener RVHs is so that other
processes will have valid window.openers.

Perhaps there's additional reasoning that makes this ok for the time being?

Today, we treat "w=window.open();w.opener=null;w.location.href=url" as a
cross-process navigation that opens in a different SiteInstance but the same
BrowsingInstance.  In that case, the opener will be suppressed and perhaps we
won't hit this logic?

For rel=noreferrer target=_blank links, we also suppress the opener and open the
link in new SiteInstance and new BrowsingInstance.  The opener won't be set
there either.

Have you tested those cases to ensure they still work?  I see a test for
rel=noreferrer, but it's not using target=_blank, so I don't think
suppress_opener will be true.

A bigger problem is that our site isolation work is making it possible for the
new window to open in a new SiteInstance even with just a normal cross-site
window.open call.  I don't want to paint us into a corner here, so I don't want
to add logic that depends on something else.  

If we do have to change something here, we should keep the process decisions in
fewer places.  The tab_util::GetSiteInstanceForNewTab function above seems like
a better place to handle it.

https://codereview.chromium.org/20924002/diff/40010/chrome/test/data/popup_bl...
File chrome/test/data/popup_blocker/popup-openersuppressed.html (right):

https://codereview.chromium.org/20924002/diff/40010/chrome/test/data/popup_bl...
chrome/test/data/popup_blocker/popup-openersuppressed.html:14: <a
href="check-openersuppressed.html" target="tab" rel="noreferrer"></a>
The process swapping logic doesn't kick in if target="tab".  It has to be
target="_blank" rel="noreferrer".  See
http://blog.chromium.org/2009/12/links-that-open-in-new-processes.html.

https://codereview.chromium.org/20924002/diff/40010/content/browser/dom_stora...
File content/browser/dom_storage/session_storage_namespace_impl.cc (right):

https://codereview.chromium.org/20924002/diff/40010/content/browser/dom_stora...
content/browser/dom_storage/session_storage_namespace_impl.cc:18:
SessionStorageNamespace::Clone(DOMStorageContext* context,
Please have Albert review this.

https://codereview.chromium.org/20924002/diff/40010/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/20924002/diff/40010/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:694: if
(params.should_stay_in_site_instance) {
I don't think we should have this, since it makes it even more difficult to tell
what SiteInstance an entry will end up with.

https://codereview.chromium.org/20924002/diff/40010/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://codereview.chromium.org/20924002/diff/40010/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager_unittest.cc:1214: // Tests
that when specified, a navigation is done in the site instance of the
I do not think we should have this behavior.

https://codereview.chromium.org/20924002/diff/40010/content/public/browser/na...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/20924002/diff/40010/content/public/browser/na...
content/public/browser/navigation_controller.h:173: bool
should_stay_in_site_instance;
We should not have this.

[jochen (gone - plz use gerrit), 2013-07-31 18:30:52]

+ajwong

Albert, can you please review the StoragePartition / SessionStorageNamespace
bits?

Basically, the situation is that we blocked a request for a new window in
RenderMessageFilter::OnCreateWindow before the session storage namespace was
cloned, and now I'd like to re-setup the creation of the new window from the
browser.

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:691:
->GetStoragePartitionIdForSite(site);
On 2013/07/31 17:16:16, creis wrote:
> nit: Arrow on previous line to be consistent (e.g., with
> SessionStorageNamespace::Clone).

Done.

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/profiles/p...
chrome/browser/profiles/profile.h:386: std::string
GetStoragePartitionIdForSite(const GURL& site);
On 2013/07/31 17:16:16, creis wrote:
> I think Albert will need to review the storage partition / session storage
> namespace changes.

Note that this particular change is just moving the code from
ChromeContentBrowserClient.

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:260:
load_url_params.should_stay_in_site_instance =
On 2013/07/31 17:16:16, creis wrote:
> This is incorrect.  Setting the opener will not always imply that you should
> stay in the same SiteInstance.  More comments below.

Removed.

https://codereview.chromium.org/20924002/diff/40010/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:331: // In order for window.opener to be
accessible, the target contents needs to
On 2013/07/31 17:16:16, creis wrote:
> This reasoning is false.  The reason that we create opener RVHs is so that
other
> processes will have valid window.openers.
> 
> Perhaps there's additional reasoning that makes this ok for the time being?
> 
> Today, we treat "w=window.open();w.opener=null;w.location.href=url" as a
> cross-process navigation that opens in a different SiteInstance but the same
> BrowsingInstance.  In that case, the opener will be suppressed and perhaps we
> won't hit this logic?
> 
> For rel=noreferrer target=_blank links, we also suppress the opener and open
the
> link in new SiteInstance and new BrowsingInstance.  The opener won't be set
> there either.
> 
> Have you tested those cases to ensure they still work?  I see a test for
> rel=noreferrer, but it's not using target=_blank, so I don't think
> suppress_opener will be true.
> 
> A bigger problem is that our site isolation work is making it possible for the
> new window to open in a new SiteInstance even with just a normal cross-site
> window.open call.  I don't want to paint us into a corner here, so I don't
want
> to add logic that depends on something else.  
> 
> If we do have to change something here, we should keep the process decisions
in
> fewer places.  The tab_util::GetSiteInstanceForNewTab function above seems
like
> a better place to handle it.

I removed the SiteInstance related logic as discussed via VC.

To summarize, by not forcing a specific site instance, we might have corner
cases where we could have a scriptable window.opener but won't have one. I guess
that's fine.

Also, not forcing the SiteInstance works fine with the current StoragePartition
model but we might need to revise that when we use more than one storage
partition. Added a comment about that.

https://codereview.chromium.org/20924002/diff/40010/chrome/test/data/popup_bl...
File chrome/test/data/popup_blocker/popup-openersuppressed.html (right):

https://codereview.chromium.org/20924002/diff/40010/chrome/test/data/popup_bl...
chrome/test/data/popup_blocker/popup-openersuppressed.html:14: <a
href="check-openersuppressed.html" target="tab" rel="noreferrer"></a>
On 2013/07/31 17:16:16, creis wrote:
> The process swapping logic doesn't kick in if target="tab".  It has to be
> target="_blank" rel="noreferrer".  See
> http://blog.chromium.org/2009/12/links-that-open-in-new-processes.html.

Done.

https://codereview.chromium.org/20924002/diff/40010/content/browser/dom_stora...
File content/browser/dom_storage/session_storage_namespace_impl.cc (right):

https://codereview.chromium.org/20924002/diff/40010/content/browser/dom_stora...
content/browser/dom_storage/session_storage_namespace_impl.cc:18:
SessionStorageNamespace::Clone(DOMStorageContext* context,
On 2013/07/31 17:16:16, creis wrote:
> Please have Albert review this.

Added Albert.

https://codereview.chromium.org/20924002/diff/40010/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/20924002/diff/40010/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:694: if
(params.should_stay_in_site_instance) {
On 2013/07/31 17:16:16, creis wrote:
> I don't think we should have this, since it makes it even more difficult to
tell
> what SiteInstance an entry will end up with.

Removed this code.

https://codereview.chromium.org/20924002/diff/40010/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://codereview.chromium.org/20924002/diff/40010/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager_unittest.cc:1214: // Tests
that when specified, a navigation is done in the site instance of the
On 2013/07/31 17:16:16, creis wrote:
> I do not think we should have this behavior.

Removed.

https://codereview.chromium.org/20924002/diff/40010/content/public/browser/na...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/20924002/diff/40010/content/public/browser/na...
content/public/browser/navigation_controller.h:173: bool
should_stay_in_site_instance;
On 2013/07/31 17:16:16, creis wrote:
> We should not have this.

Removed.

[awong, 2013-07-31 18:58:59]

You're gonna hate my answer...but I think we should actually remove
SessionStorageNamespaceMap first.

https://codereview.chromium.org/20924002/diff/84001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/20924002/diff/84001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:342: // determine the storage partition
is always ok.
This can never happen now. Each RenderViewHost should only ever have 1
StoragePartitionNamespace so you should be able to just clone it.

https://codereview.chromium.org/20924002/diff/84001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:359:
namespace_map.insert(make_pair(partition_id, cloned_namespace));
We should remove the SessionStorageNamespaceMap, which should remove the need to
expose a partition_id.

https://codereview.chromium.org/20924002/diff/84001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.h (right):

https://codereview.chromium.org/20924002/diff/84001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.h:215: // Indicates whether the session
storage namespace map of |source_contents|
I think there's no map anymore...

[jochen (gone - plz use gerrit), 2013-07-31 20:18:38]

As discussed, here's a CL that doesn't muck with session storage at all and does
a best effort to set the opener.

Note that I'm setting a default parameter in web_contents.h to ease reviewing.
If you like the patch, I'll remove the default parameter and update all ~60 call
sites to pass NULL explicitly.

[jochen (gone - plz use gerrit), 2013-07-31 20:18:58]

(I'll also update the CL description)

[awong, 2013-07-31 20:21:41]

https://codereview.chromium.org/20924002/diff/89002/chrome/test/data/popup_bl...
File chrome/test/data/popup_blocker/check-sessionstorage.html (right):

https://codereview.chromium.org/20924002/diff/89002/chrome/test/data/popup_bl...
chrome/test/data/popup_blocker/check-sessionstorage.html:4: <title>Check that
the session storage namespace is correct</title>
is accessible is maybe better?

https://codereview.chromium.org/20924002/diff/89002/content/public/browser/we...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/20924002/diff/89002/content/public/browser/we...
content/public/browser/web_contents.h:97: WebContents* opener = NULL);
default args aren't allowed by style guide.  Maybe add a CreateWithOpener()

[jochen (gone - plz use gerrit), 2013-07-31 20:23:43]

https://codereview.chromium.org/20924002/diff/89002/content/public/browser/we...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/20924002/diff/89002/content/public/browser/we...
content/public/browser/web_contents.h:97: WebContents* opener = NULL);
On 2013/07/31 20:21:42, awong wrote:
> default args aren't allowed by style guide.  Maybe add a CreateWithOpener()

The default arg is just there to make it possible to find the actual code to
review. I'll remove the default parameter if you like the actual code.

[awong, 2013-07-31 20:27:06]

session storage bits (which are mostly gone) LGTM

As for default args, here's the style guide rule:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

I'll defer to the rest of you on what's the right thing to do here.  That way
Jochen can deal with 1 less reviewer. :)



On 2013/07/31 20:23:43, jochen wrote:
>
https://codereview.chromium.org/20924002/diff/89002/content/public/browser/we...
> File content/public/browser/web_contents.h (right):
> 
>
https://codereview.chromium.org/20924002/diff/89002/content/public/browser/we...
> content/public/browser/web_contents.h:97: WebContents* opener = NULL);
> On 2013/07/31 20:21:42, awong wrote:
> > default args aren't allowed by style guide.  Maybe add a CreateWithOpener()
> 
> The default arg is just there to make it possible to find the actual code to
> review. I'll remove the default parameter if you like the actual code.

[Charlie Reis, 2013-08-01 18:13:33]

Much simpler-- I like it.  I think this approach (not trying to guarantee that
the popup is in the same process as the opener) is much saner, given that the
opener gets back null from window.open and can't easily script the new window
anyway.

Same goes for not cloning the session storage namespace, since we would be
cloning it at the wrong point in time (after the opener could have made changes
to its own namespace).

Basically, if we're ok with breaking scripting of popups in the first place,
then we shouldn't worry about extra complexity for these cases.

Just a few suggestions below.

https://codereview.chromium.org/20924002/diff/89002/chrome/browser/ui/blocked...
File chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc (right):

https://codereview.chromium.org/20924002/diff/89002/chrome/browser/ui/blocked...
chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc:112:
nav_params.should_set_opener = !opener_suppressed;
What if we just had a nav_params.opener_contents that we set to web_contents()
if !opener_suppressed?  Then we don't need to change the WebContents::Create
API.

https://codereview.chromium.org/20924002/diff/89002/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.h (right):

https://codereview.chromium.org/20924002/diff/89002/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.h:92: content::WebContents* source_contents;
Perhaps add content::WebContents* opener_contents here and remove
should_set_opener below?

https://codereview.chromium.org/20924002/diff/89002/chrome/test/data/popup_bl...
File chrome/test/data/popup_blocker/check-sessionstorage.html (right):

https://codereview.chromium.org/20924002/diff/89002/chrome/test/data/popup_bl...
chrome/test/data/popup_blocker/check-sessionstorage.html:4: <title>Check that
the session storage namespace is correct</title>
On 2013/07/31 20:21:42, awong wrote:
> is accessible is maybe better?

Not accessible, right?  The test below is making sure you can't access the
namespace of the opener tab.

https://codereview.chromium.org/20924002/diff/89002/chrome/test/data/popup_bl...
File chrome/test/data/popup_blocker/popup-sessionstorage.html (right):

https://codereview.chromium.org/20924002/diff/89002/chrome/test/data/popup_bl...
chrome/test/data/popup_blocker/popup-sessionstorage.html:4: Check that the
session storage namespace is correctly set when opening a
is correctly set -> of the parent is not accessible (?)

Mainly, it's not clear what "correctly set" means.

[jochen (gone - plz use gerrit), 2013-08-01 19:48:51]

PTAL

https://codereview.chromium.org/20924002/diff/89002/chrome/browser/ui/blocked...
File chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc (right):

https://codereview.chromium.org/20924002/diff/89002/chrome/browser/ui/blocked...
chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc:112:
nav_params.should_set_opener = !opener_suppressed;
On 2013/08/01 18:13:34, creis wrote:
> What if we just had a nav_params.opener_contents that we set to web_contents()
> if !opener_suppressed?  Then we don't need to change the WebContents::Create
> API.

chrome::NavigateParams already has source_contents, so I'd say a bool is enough
here. Changing WebContents::CreateParams is a good idea, I will do that.

https://codereview.chromium.org/20924002/diff/89002/chrome/test/data/popup_bl...
File chrome/test/data/popup_blocker/check-sessionstorage.html (right):

https://codereview.chromium.org/20924002/diff/89002/chrome/test/data/popup_bl...
chrome/test/data/popup_blocker/check-sessionstorage.html:4: <title>Check that
the session storage namespace is correct</title>
On 2013/08/01 18:13:34, creis wrote:
> On 2013/07/31 20:21:42, awong wrote:
> > is accessible is maybe better?
> 
> Not accessible, right?  The test below is making sure you can't access the
> namespace of the opener tab.

Done.

https://codereview.chromium.org/20924002/diff/89002/chrome/test/data/popup_bl...
File chrome/test/data/popup_blocker/popup-sessionstorage.html (right):

https://codereview.chromium.org/20924002/diff/89002/chrome/test/data/popup_bl...
chrome/test/data/popup_blocker/popup-sessionstorage.html:4: Check that the
session storage namespace is correctly set when opening a
On 2013/08/01 18:13:34, creis wrote:
> is correctly set -> of the parent is not accessible (?)
> 
> Mainly, it's not clear what "correctly set" means.

Done.

[Charlie Reis, 2013-08-01 20:23:08]

Thanks.  LGTM.

https://codereview.chromium.org/20924002/diff/104001/chrome/browser/ui/blocke...
File chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc (right):

https://codereview.chromium.org/20924002/diff/104001/chrome/browser/ui/blocke...
chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc:245: // If the
popup blocker blocked the window.open, there should be only one
nit: s/If/Since/

https://codereview.chromium.org/20924002/diff/104001/chrome/test/data/popup_b...
File chrome/test/data/popup_blocker/popup-sessionstorage.html (right):

https://codereview.chromium.org/20924002/diff/104001/chrome/test/data/popup_b...
chrome/test/data/popup_blocker/popup-sessionstorage.html:4: Check that the
session storage namespace is not accessible blocked popup
nit: not accessible to a blocked popup

[jochen (gone - plz use gerrit), 2013-08-01 20:31:53]

done

[commit-bot: I haz the power, 2013-08-01 20:32:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/20924002/120001

[jochen (gone - plz use gerrit), 2013-08-01 21:49:22]

Committed patchset #11 manually as r215122 (presubmit successful).