GuestView needs to support window.open + write + redirect via BeginNavigation |
||||
Issue descriptionIn 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?
,
Oct 4
I think I may have a fix for this in the WIP CL @ https://crrev.com/c/1262089?
,
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
,
Oct 18
,
Oct 19
|
||||
►
Sign in to add a comment |
||||
Comment 1 by creis@chromium.org
, Sep 14