1999481 - Downloads triggered by a link with target="_blank" are not linked to their navigation

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P2)

Description

[Holger Benl [:hbenl]]

When a download is triggered by a click on a link with target="_blank", the navigation property of the download events is set to null even though the click does trigger a browsingContext.navigationStarted event.
This can be reproduced by modifying this test:

• add target="_blank" to this link
• running the test now fails here because navigation is None
• replace any_string with None here
• now the test fails here, showing that the test does receive a browsingContext.navigationStarted event.

Comment 1

[Julian Descottes [:jdescottes]]

Good catch. We match downloadWillBegin with a navigation based on the fact that they have the same navigable id: https://searchfox.org/firefox-main/rev/c82adde571c24d89da3ef13f11245c8f572ba1e6/remote/shared/NavigationManager.sys.mjs#762-785

#onDownloadStarted = (eventName, data) => {
  const { download } = data;

  const contextId = download.source.browsingContextId;
  const browsingContext =
    lazy.NavigableManager.getBrowsingContextById(contextId);
  if (!browsingContext) {
    return;
  }

  const navigableId =
    lazy.NavigableManager.getIdForBrowsingContext(browsingContext);
  const url = download.source.url;

  const navigation = this.#navigations.get(navigableId);
  let navigationId = null;
  if (navigation && navigation.state === NavigationState.Started) {
    // navigationId is optional and should only be set if there is an ongoing
    // navigation.
    navigationId = navigation.navigationId;
    // Track the navigation id for this download object, for the upcoming
    // NAVIGATION_EVENTS.DownloadEnd event.
    this.#downloadNavigations.set(download, navigationId);
  }

In this case, the download is registered for the browsing context where the link is located, but the navigation is registered for another (temporary?) context.

Comment 2

[Holger Benl [:hbenl]]

This Playwright test is failing due to this issue.

What I found in the HTML spec:
The Beginning Navigation algorithm creates a navigationId in step 7. and passes it to Attempt to populate the history entry's document in step 23.9. That algorithm passes the navigationId to Handle as a download in step 5.5.5.1. and that algorithm sets the navigationId in the BiDi download events.

However, when following this path of the spec, the navigable for the BiDi navigation started event (step 17. in Beginning Navigation) is the same as in the download events. I couldn't find anything in the spec that "retargets" the download from a popup's initial navigation to the popup's opener, which is what Playwright expects here.

Comment 3

[Alexandra Borovova [:Sasha]]

Ok, so reading the spec, the most reasonable thing for me was to map the downloadWillBegin to the navigationStarted event, so use the same navigationId as the navigation from the temporary browsing context. As Holger said, there is no retargeting, so the spec implies that we should map the download to the navigation (even if it happens in the temporary browsing context). We can do this by checking the opener of the browsing context that started navigation (comparing it to the browsing context that triggers download). With the fix, the mentioned Playwright test started passing.
BUT now running the tests in CI it looks like we cannot guarantee that we will detect this navigation (e.g., https://treeherder.mozilla.org/logviewer?job_id=549729753&repo=try&task=MZN-Wqy5Qz-RphrWL8Wnfw.0&lineNumber=6837, there is no navigationStarted event). From the download event, as far as I know, we cannot understand that it came not from the link, so we cannot just create a new navigation if it's missing.
We could theoretically create navigations for all downloads (if it's missing), but as far as I know, that's not what we want. So I guess we need something to tell us where the download comes from.

Comment 4

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1999481 - Add "triggeredByContentDispositionHeader" to download.source to allow identifying that the download was triggered by the request with the "Content-Disposition" header.

Comment 5

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1999481 - [webdriver-bidi] Start the navigation in the same browsing context for "Content-Disposition" downloads, when the navigation is missing.

Comment 6

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1999481 - [wdspec] Add test cases for "browsingContext.downloadWillBegin" and "browsingContext.downloadEnd" that triggered by "Content-Disposition" header in the new tab.

Comment 7

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1999481 - [wdspec] Update tests for download events from "Content-Disposition" header to take into account possible navigation event in temporary context.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0e331ae15bda
https://hg.mozilla.org/integration/autoland/rev/897dc490588c
Add "triggeredByContentDispositionHeader" to download.source to allow identifying that the download was triggered by the request with the "Content-Disposition" header. r=mak
https://github.com/mozilla-firefox/firefox/commit/bda9e4753963
https://hg.mozilla.org/integration/autoland/rev/b69126a93461
[webdriver-bidi] Start the navigation in the same browsing context for "Content-Disposition" downloads, when the navigation is missing. r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/94aae9afe408
https://hg.mozilla.org/integration/autoland/rev/c8432f0bfa4c
[wdspec] Add test cases for "browsingContext.downloadWillBegin" and "browsingContext.downloadEnd" that triggered by "Content-Disposition" header in the new tab. r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/a0ac50f7a77f
https://hg.mozilla.org/integration/autoland/rev/b723de799a41
[wdspec] Update tests for download events from "Content-Disposition" header to take into account possible navigation event in temporary context. r=jdescottes

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/58371 for changes under testing/web-platform/tests

Comment 10

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/897dc490588c
https://hg.mozilla.org/mozilla-central/rev/b69126a93461
https://hg.mozilla.org/mozilla-central/rev/c8432f0bfa4c
https://hg.mozilla.org/mozilla-central/rev/b723de799a41

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot