New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 884383 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 18
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 866274



Sign in to add a comment

GuestView needs to support window.open + write + redirect via BeginNavigation

Project Member Reported by creis@chromium.org, Sep 14

Issue description

In https://chromium-review.googlesource.com/c/chromium/src/+/1188643, timothygu@ and alexmos@ spotted a window.open case where GuestViews seem to depend on the old OpenURL path in the renderer rather than the BeginNavigation path.

More specifically, in the WebViewNewWindowInteractiveTest.NewWindow_Redirect test in interactive_ui_tests, a <webview> tries to open a new window using this approach:

        var w = window.open('', frameName);
        w.opener = null;
        w.document.write(
            '<META HTTP-EQUIV="refresh" content="0; url=' + url + '">');
        w.document.close();

That models what Gmail was doing in issue 233475.  The "w=window.open();w.opener=null;navigate" part triggers the "Gmail fork heuristic" where OpenURL is called, and the document.write + meta-refresh part is what Gmail was using to navigate.

Fady fixed this case for <webview>, but Alex found that it seems to depend on the OpenURL path.  We're moving away from that path in general now that PlzNavigate has shipped, and timothygu@'s https://chromium-review.googlesource.com/c/chromium/src/+/1188643 happened to also change the document.write behavior so that OpenURL also doesn't kick in (since the old_url in decidePolicyForNavigation no longer looks like about:blank after document.write, basically undoing the fix for  issue 93517 ).

We should update GuestView to support opening windows in this way using BeginNavigation instead.  As Alex notes:

https://chromium-review.googlesource.com/c/chromium/src/+/1188643/8/chrome/test/data/extensions/platform_apps/web_view/newwindow/guest_opener.html#30
---
In the case with BeginNavigation, the navigation gets canceled by a new about:blank that originates from here in WebViewGuest::ApplyAttributes: https://cs.chromium.org/chromium/src/extensions/browser/guest_view/web_view/web_view_guest.cc?l=1154&rcl=630ab6596e3620e984fe6a45e82a23b07600b5c4

Looks like that function doesn't read the correct URL from pending_new_windows_, and indeed, it depends on the OpenURL branch putting it there in the first place, via WebViewGuest::OpenURLFromTab -> WebViewGuest::CreateNewGuestWebViewWindow -> WebViewGuest::NewGuestWebViewCallback.  So it does appear that window.open from a <webview> is only supported via OpenURL.  I think it's probably possible to fix this to also work with the PlzNavigate path (BeginNavigation)
---

We'd like to proceed with Timothy's CL, so hopefully we can fix this in parallel in M71 to avoid a possible regression.  James, can you help triage and find an owner?
 
Cc: domenic@chromium.org
Cc: wjmaclean@chromium.org
Owner: lukasza@chromium.org
Status: Started (was: Untriaged)
I think I may have a fix for this in the WIP CL @ https://crrev.com/c/1262089?
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0962f52f2b0d1627ee5b9446ec9ce79e275c428d

commit 0962f52f2b0d1627ee5b9446ec9ce79e275c428d
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Oct 17 17:56:54 2018

Remove non-standard process fork heuristic used by desktop Gmail.

Setting a new window's opener to null and navigating it cross-site used
to be a hint to Chrome that it was safe to open the link in a new
process.  Now that Site Isolation is enabled by default on desktop,
cross-site links will always open in a new process, making this
unnecessary there.

For cases where Site Isolation is not enabled, it is better to use
target=_blank rel=noreferrer (or rel=noopener) than this old,
non-standard heuristic.  See:
https://blog.chromium.org/2009/12/links-that-open-in-new-processes.html

Bug: 892212,  892375 ,  884383 
Change-Id: I50422cb6bd97f8e0f1d4ef082551ecb355ae104b
Reviewed-on: https://chromium-review.googlesource.com/c/1262089
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Ɓukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600469}
[modify] https://crrev.com/0962f52f2b0d1627ee5b9446ec9ce79e275c428d/content/renderer/render_frame_impl.cc
[delete] https://crrev.com/382ffc533cb64b0946adfbc617858a6203207d03/content/test/data/fork-popup.html
[modify] https://crrev.com/0962f52f2b0d1627ee5b9446ec9ce79e275c428d/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/0962f52f2b0d1627ee5b9446ec9ce79e275c428d/extensions/browser/guest_view/web_view/web_view_guest.h
[modify] https://crrev.com/0962f52f2b0d1627ee5b9446ec9ce79e275c428d/third_party/WebKit/LayoutTests/TestExpectations

Status: Fixed (was: Started)
Blocking: 866274

Sign in to add a comment