Partially revert r280256 behavior change since it broke Facetime
Created attachment 435242 [details] Patch
<rdar://problem/81700268>
Comment on attachment 435242 [details] Patch r=me
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.
I'll also revert two tests because the semantics is reverted now.
Created attachment 435244 [details] Patch
Created attachment 435245 [details] Patch
Committed r280826 (240389@main): <https://commits.webkit.org/240389@main>
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.
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.
(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.
(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.
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
Got it. Thanks for the explanation!
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.
(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).
(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.