Bug 228943 – Partially revert r280256 behavior change since it broke Facetime

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

[Yusuke Suzuki]

Partially revert r280256 behavior change since it broke Facetime

[Yusuke Suzuki]

Created attachment 435242 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/81700268>

[Mark Lam]

Comment on attachment 435242 [details]
Patch

r=me

[Yusuke Suzuki]

If we perform `location.replace` from iframe for the top-level window with app link (facetime-open-link://...), before this change, it prompted the window to open it in app. But now, this won't happen.

[Yusuke Suzuki]

I'll also revert two tests because the semantics is reverted now.

[Yusuke Suzuki]

Created attachment 435244 [details]
Patch

[Yusuke Suzuki]

Created attachment 435245 [details]
Patch

[Yusuke Suzuki]

Committed r280826 (240389@main): <https://commits.webkit.org/240389@main>

[Geoffrey Garen]

Comment on attachment 435245 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        We revert the semantic change by using ActiveWindow for replace, assign, and reload. This is
> +        not the correct semantics from the spec, but this is the same to one before r280256, and we
> +        will change this once we find the way to bypass this issue.

Are you sure this is not the correct semantics?

My understanding from recently reading the spec for the Location object is that it is very special, and it uses a different window than most APIs use, for backwards-compatibility.

[Geoffrey Garen]

Specifically, URL parsing is specified to use the entry settings object, which is the the *bottom* of the JS stack, where execution began:

> The href attribute's setter must run these steps:

> If this Location object's relevant Document is null, then return.

> Parse the given value relative to the entry settings object. If that failed, throw a TypeError exception.

> Location-object navigate given the resulting URL record.

[Alexey Shvayka]

(In reply to Geoffrey Garen from comment #10)
> Specifically, URL parsing is specified to use the entry settings object,
> which is the the *bottom* of the JS stack, where execution began:

Yeah, that's why FirstWindow (which is implemented by VMEntryScope) is passed: to complete the URL.

IncumbentWindow comes into play a little later: please see step 2 of https://html.spec.whatwg.org/multipage/history.html#location-object-navigate, for security checks and `document.referrer`.

It's normative description (https://html.spec.whatwg.org/multipage/webappapis.html#topmost-script-having-execution-context) is precisely what CallFrame::globalObjectOfClosestCodeBlock() implements: closest (in stack) userland caller function or module / global code.

Also, adjusted LayoutTests were matching Gecko and Blink before r280826.

With high degree of certainty, I believe using IncumbentWindow is the correct semantics, which was temporarily reverted until we figure out why FaceTime is broken.

[Alexey Shvayka]

(In reply to Geoffrey Garen from comment #9)
> My understanding from recently reading the spec for the Location object is
> that it is very special, and it uses a different window than most APIs use,
> for backwards-compatibility.

That's true, most APIs use ActiveWindow, but Location is special.

[Yusuke Suzuki]

Comment on attachment 435245 [details]
Patch

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

>> Source/WebCore/ChangeLog:19
>> +        will change this once we find the way to bypass this issue.
> 
> Are you sure this is not the correct semantics?
> 
> My understanding from recently reading the spec for the Location object is that it is very special, and it uses a different window than most APIs use, for backwards-compatibility.

Yes, Location's spec states that we need to use ImcumbentWindow (while the usual APIs typically use ActiveWindow).
https://html.spec.whatwg.org/multipage/history.html#location-object-navigate

[Geoffrey Garen]

Got it. Thanks for the explanation!

[Geoffrey Garen]

I wonder if the incompatibility is caused by the fact that DOMWindow::setLocation expects to be passed the active window:

    void DOMWindow::setLocation(DOMWindow& activeWindow, const URL& completedURL, SetLocationLocking locking)

Since the passed-in window is the window that we will navigate, it seems appropriate to use the active window (the callee's window) rather than the incumbent window (the caller's window).

Side note: The spec name for 'active window' appears to be 'current window': https://html.spec.whatwg.org/#concept-current-everything.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #15)
> I wonder if the incompatibility is caused by the fact that
> DOMWindow::setLocation expects to be passed the active window:
> 
>     void DOMWindow::setLocation(DOMWindow& activeWindow, const URL&
> completedURL, SetLocationLocking locking)
> 
> Since the passed-in window is the window that we will navigate, it seems
> appropriate to use the active window (the callee's window) rather than the
> incumbent window (the caller's window).
> 
> Side note: The spec name for 'active window' appears to be 'current window':
> https://html.spec.whatwg.org/#concept-current-everything.

I am with Geoff on this, I think we're simply navigating the wrong window. As per:

- https://html.spec.whatwg.org/#dom-location-href
We use the first (aka entry) window for relative URL resolution.

- https://html.spec.whatwg.org/#location-object-navigate
We navigate the current (aka active) window

- https://html.spec.whatwg.org/#navigate
We use the incumbent window for the security check (allowed to navigate).

[Alexey Shvayka]

(In reply to Geoffrey Garen from comment #15)
> I wonder if the incompatibility is caused by the fact that
> DOMWindow::setLocation expects to be passed the active window:
> 
>     void DOMWindow::setLocation(DOMWindow& activeWindow, const URL&
> completedURL, SetLocationLocking locking)
> 
> Since the passed-in window is the window that we will navigate, it seems
> appropriate to use the active window (the callee's window) rather than the
> incumbent window (the caller's window).

Please note that DOMWindow::setLocation() navigates this->frame(), while first parameter is used for security checks, outgoingReferrer(), and as initiating document for NavigationScheduler::scheduleLocationChange() (all seems to be according to https://html.spec.whatwg.org/#navigate).

this->frame() here would be the same as ActiveWindow, so I guess we navigate the correct window. Otherwise, we would see a lot of test failures.

However, it might be that we are overusing the first parameter of setLocation(); maybe it's something related to navigation policy. I will keep digging and keep you posted.