Bug 220658 – A DOMWindow should not be reused if its JSDOMWindow has been created

• Component: Page Loading
• Status: RESOLVED FIXED

[youenn fablet]

WindowProxy should be updated when the DOMWindow is transitioned from one document to another

[youenn fablet]

<rdar://problem/70335075>

[youenn fablet]

Created attachment 417715 [details]
WIP

[youenn fablet]

One repro step for the problem:
- Update MobileMiniBrowser to be limitsNavigationsToAppBoundDomains and add webkit.org  to the allowed list
- Add a navigation delegate to block about:blank and instead make it load another secure domain than webkit.org.
- Load webkit.org and attach an inspector to it
- Through inspector, trigger loading to about:blank
- This should go to the other domain page.

At that point, the JSDOMWindow will be created before having the document with the secure context. Window properties that are secure will not show up.

I am not sure how to further reduce the test case.
It seems this is triggered by limitsNavigationsToAppBoundDomains so I am unsure whether this is the exact right fix.

[Geoffrey Garen]

I'm not sure why limitsNavigationsToAppBoundDomains specifically triggers this; it seems like any app or extension use of JavaScript in the page could potentially trigger this.

Code change seems OK to me; but EWS is angry.

[youenn fablet]

(In reply to Geoffrey Garen from comment #4)
> I'm not sure why limitsNavigationsToAppBoundDomains specifically triggers
> this; it seems like any app or extension use of JavaScript in the page could
> potentially trigger this.

Right, my testing was wrong without limitsNavigationsToAppBoundDomains.
The bug can be done with early JS injection, prior to the document having its security origin and URL set. Web Inspector also triggers this.

[youenn fablet]

> Code change seems OK to me; but EWS is angry.

We need to handle the window.open in same process case where the window document secure context is set by its opener, hence the wrapping is done correctly.

[youenn fablet]

Created attachment 417810 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #7)
> Created attachment 417810 [details]
> Patch

Patch covers the case of isSecureContext.
Patch also covers most of the Quirked properties as well, but it is difficult to ensure this in the future as this might depend on how quirks will implement their checks.

[youenn fablet]

Comment on attachment 417810 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEvaluateJavaScript.mm:822
> +    [webView stringByEvaluatingJavaScript:@""];

Maybe there is a known pattern, but it might be difficult for developers to know when they can evaluate JS in a page being loaded, before page JS starts executing but after the DOMWindow has its final document.
Maybe we should consider delaying evaluation of this JS a bit so that the DOMWindow would have a correct SecurityOrigin.
Also, the transition of the window to a new document is clearing location or navigator, which might have subtle and potentially racy effects.

[Geoffrey Garen]

Comment on attachment 417810 [details]
Patch

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

r=me

> Source/WebCore/page/DOMWindow.h:487
> +    bool m_wasWrappedWithoutInitializedDocument { false };

I would call this wasWrappedWithoutInitializedSecurityOrigin, since it's the security origin that was not initialized.

[youenn fablet]

Created attachment 417950 [details]
Patch for landing

[EWS]

Committed r271642: <https://trac.webkit.org/changeset/271642>

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