New issue
Advanced search Search tips

Issue 881819 link

Starred by 1 user

Issue metadata

Status: ExternalDependency
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: 2019-03-01
OS: iOS
Pri: 3
Type: Bug



Sign in to add a comment

Certain window.open(...).close() call corrupts window.localStorage on Chrome 69 in iOS

Reported by mfaust...@gmail.com, Sep 7

Issue description

Steps to reproduce the problem:
1. Load the attached html.
2. Click the "Break Storage" button.
3. reload the page.

What is the expected behavior?
The page should produce an alert both times with the content of window.localStorage. If there is nothing in localStorage then "{}" should appear in the alert both times.

What went wrong?
The localStorage gets corrupted and "null" is returned for window.localStorage.

Did this work before? N/A 

Chrome version: 69.0.3497.81  Channel: stable
OS Version: 10.3.3
Flash Version:
 
close.html
455 bytes View Download
Components: -Blink
Not blink
Cc: mrefaat@chromium.org
Components: Mobile>WebView>Glue
Owner: eugene...@chromium.org
Status: Assigned (was: Unconfirmed)
Please investigate.
Is this problem reproducible with Safari and/or Firefox?
Problem does NOT happen on Firefox and Safari on iOS.

Problem DOES happen on Edge and Opera Mini on iOS.
Sorry. Previous comment was for a different bug. Below are the behaviours of other browsers on iOS:

Problem does NOT happen on Firefox, Opera Mini, or Safari on iOS.

Problem DOES happen on Edge and Chrome on iOS.
Components: Mobile>iOSWeb
Components: -Mobile>WebView>Glue
Cc: ajuma@chromium.org
Components: -Mobile>iOSWeb Mobile>iOSWeb>WebPlatform
I took a quick a look at this. 

We're getting a null value for window.localStorage in WebKit's DOMWindow::localStorage because page->isClosing() is true. And that's because the close() call in script is closing the DOMWindow, which in turn puts the Page into a closing state.

In Safari, the view does close and we get take back to the new tab page.

In Chromium, we get a call to [CRWWebController webViewDidClose] but we ignore it since self.hasOpener is false: https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?rcl=e6f134abefe58874b15d44caf07d0e3f3fb39ec9&l=4249

So we continue to allow the user to interact with a WKWebView that should really be closed. If we ignore the value of |hasOpener|, we close the tab and get correct behavior.

So I think the underlying bug is having an incorrect |hasOpener| in this situation.

Thanks Ali. I wonder how did we end up with false value for |hasOpener| property.


webView:createWebViewWithConfiguration:forNavigationAction:windowFeatures: actually DCHECKs that |hasOpener| returns true:
DCHECK(!childWebController || childWebController.hasOpener);

And there is no code on iOS which would change this boolean to false (SetHasOpener is only called from CWVWebView class, which is not used in Chrome).
The sample project contains the following code:

|window.open('#','_self').close();|

In Safari this closes the window. In Chrome (both Desktop and iOS) close does nothing, because the page does not have an opener. 

Ali, do you know if spec says anything about allowing to close the window without opener?
The spec does indeed say that we should not allow scripts to close a window that wasn't opened by script (unless there's the window has just a single item in its back/forward history):
https://html.spec.whatwg.org/multipage/window-object.html#script-closable

Debugging some more, it turns out that WebKit has a bug in DOMWindow::createWindow where doing |window.open('','_self')| sets the window as having been opened by script, so WebKit allows window.close() on that window.

I'll follow up with trying to get this fixed in WebKit.

That said, aside from this bug, WebKit does properly disallow window.close() for windows that weren't opened from script, so we shouldn't need to duplicate that check in Chrome when we get a webViewDidClose callback.
WKWebView does not provide API to restore |hasOpener| flag across cold launches. This is why we need |hasOpener| check in chrome. Given that WKWebView does not allow to restore |hasOpener|, we still need webViewDidClose: callback to be called even for windows which do not have an opener. Is it possible to fix window.localStorage corruption, but still call webViewDidClose: callback?
I'm not sure I follow -- is the scenario that we want to restore a |true| value for |hasOpener|? But then we have:
-We restore a page and set hasOpener to be true.
-But as far as WebKit is concerned, the page has no opener.
-So WebKit disallows window.close(), and we never even see webViewDidClose.

In this case, the script that called window.close needs to immediately see that the call failed, so there's no time to wait for an embedder callback.

Or is the bug that we want to restore a |false| value, and WebKit thinks the page does have an opener?

For the specific question about window.localStorage, WebKit did consider allowing this to work on closed windows (https://bugs.webkit.org/show_bug.cgi?id=135330) four years ago, but dropped it since there didn't seem to be a need for it.
This is the former. 

-We restore a page and set hasOpener to be true.
-But as far as WebKit is concerned, the page has no opener.
-So WebKit disallows window.close(), and we never even see webViewDidClose.

I've filed a WebKit bug and created a patch for the window.open('#', '_self') issue: https://bugs.webkit.org/show_bug.cgi?id=191073

I'm curious to learn more about how openers are supposed to work when restoring across a cold launch. For example, how the script that previously opened the window re-gains access to the new window. But maybe that's easier to discuss in a 1:1 than in a bug.
Cc: eugene...@chromium.org
Labels: -Pri-2 Pri-3
Owner: ----
Status: ExternalDependency (was: Assigned)
Thank you for submitting the fix.

Short answer to #15: opener window does not regain access to opened window. But window.close() still works properly, which is good.
The WebKit patch for this has landed: https://trac.webkit.org/r237598
NextAction: 2019-03-01
Setting a NextAction date to verify the fix after it makes it into an iOS release.

Sign in to add a comment