Bug 218804 – sessionStorage should not be cloned when a window is opened with rel=noopener

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Lawrence (MSFT)]

WebKit should not copy session storage when a new window opens with rel="noopener".

https://html.spec.whatwg.org/multipage/browsers.html#copy-session-storage

Firefox and the Web Platform tests (wpt/webstorage/storage_session_window_noopener) get this right; Safari and Chrome (https://crbug.com/771959) get this wrong. 

I'd like to fix this for Chrome, and getting it fixed in Safari will help.

Demo/Repro:
1. Visit https://webdbg.com/test/sessions/
2. Pick a color in the dropdown. Observe SessionStorage value shown in page.
3. Click 'open this page in ... noopener' link

OBSERVE: The new page shows the sessionStorage value was copied from the original window.

[Radar WebKit Bug Importer]

<rdar://problem/71286606>

[John Wilander]

Thanks for filing, Eric! Cc’ing Sihui.

[Eric Lawrence (MSFT)]

This change has landed in Chromium 89, slated for stable release around March 2021.

[Chris Dumez]

(In reply to Eric Lawrence (MSFT) from comment #3)
> This change has landed in Chromium 89, slated for stable release around
> March 2021.

Thanks for the info. I have it implemented, just working on tests now.

[Chris Dumez]

Created attachment 415047 [details]
Patch

[Alex Christensen]

Comment on attachment 415047 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415047&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/SessionStorage.mm:54
> +- (void)webView:(WKWebView *)webView runJavaScriptConfirmPanelWithMessage:(NSString *)message initiatedByFrame:(WKFrameInfo *)frame completionHandler:(void (^)(BOOL result))completionHandler

TestUIDelegate has waitForAlert.  We could so something similar instead of making another global done boolean that prevents us from unifying sources.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/SessionStorage.mm:67
> +    sharedUIDelegate = adoptNS([[SessionStorageUIDelegate alloc] init]);

This could just be a function-scoped static variable.

[Chris Dumez]

Comment on attachment 415047 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415047&action=review

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/SessionStorage.mm:54
>> +- (void)webView:(WKWebView *)webView runJavaScriptConfirmPanelWithMessage:(NSString *)message initiatedByFrame:(WKFrameInfo *)frame completionHandler:(void (^)(BOOL result))completionHandler
> 
> TestUIDelegate has waitForAlert.  We could so something similar instead of making another global done boolean that prevents us from unifying sources.

I wasn't familiar this TestUIDelegate's waitForAlert. Seems trivial to use it for the main WebView. However, how would I use it for the new webview that gets opened?

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/SessionStorage.mm:67
>> +    sharedUIDelegate = adoptNS([[SessionStorageUIDelegate alloc] init]);
> 
> This could just be a function-scoped static variable.

I don't think so? It is used in createWebViewWithConfiguration above to set the delegate on the new webView. We use the same UIDelegate for the new WebView.

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 415047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415047&action=review
> 
> >> Tools/TestWebKitAPI/Tests/WebKitCocoa/SessionStorage.mm:54
> >> +- (void)webView:(WKWebView *)webView runJavaScriptConfirmPanelWithMessage:(NSString *)message initiatedByFrame:(WKFrameInfo *)frame completionHandler:(void (^)(BOOL result))completionHandler
> > 
> > TestUIDelegate has waitForAlert.  We could so something similar instead of making another global done boolean that prevents us from unifying sources.
> 
> I wasn't familiar this TestUIDelegate's waitForAlert. Seems trivial to use
> it for the main WebView. However, how would I use it for the new webview
> that gets opened?
> 
> >> Tools/TestWebKitAPI/Tests/WebKitCocoa/SessionStorage.mm:67
> >> +    sharedUIDelegate = adoptNS([[SessionStorageUIDelegate alloc] init]);
> > 
> > This could just be a function-scoped static variable.
> 
> I don't think so? It is used in createWebViewWithConfiguration above to set
> the delegate on the new webView. We use the same UIDelegate for the new
> WebView.

Also TestUIDelegate does not seem to implement createWebViewWithConfiguration.

[Alex Christensen]

It also doesn't implement runJavaScriptConfirmPanelWithMessage but it could.

[Chris Dumez]

(In reply to Alex Christensen from comment #9)
> It also doesn't implement runJavaScriptConfirmPanelWithMessage but it could.

It could, but how about the other issues I mentioned? How do I use [TestUIDelegate waitForAlert:] for the newly opened window? Seems like it would be racy. I can wait for openedWebView to become non-null. But then calling [openedWebView.UIDelegate waitForAlert] seems like it would be racy because it is possible the alert was already shown by this point.

[Alex Christensen]

These are just nits about the test variable scopes.  Feel free to commit as-is.

sharedUIDelegate would need its own function to be function scoped and shared between two other functions.

The main test may need to wait for createWebViewWithConfiguration to be called then wait for a message after that on the UIDelegate that is set.  Could be done.

[Chris Dumez]

Created attachment 415056 [details]
Patch

[Chris Dumez]

(In reply to Alex Christensen from comment #11)
> These are just nits about the test variable scopes.  Feel free to commit
> as-is.
> 
> sharedUIDelegate would need its own function to be function scoped and
> shared between two other functions.
> 
> The main test may need to wait for createWebViewWithConfiguration to be
> called then wait for a message after that on the UIDelegate that is set. 
> Could be done.

I think this latest patch iteration addresses your comments. Let me know if you think it can still be improved.

[EWS]

Committed r270273: <https://trac.webkit.org/changeset/270273>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415056 [details].