2005963 - downloads.download() unnecessarily requires an active browser window

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[Rob Wu [:robwu]]

As part of fixing bug 2005953, I created a xpcshell test to serve as a regression test. The test calls browser.downloads.download() with saveAs:true to open a file picker, but that API call failed because xpcshell tests do not have browser windows, but the ext-downloads.js implementation looks up one at https://searchfox.org/firefox-main/rev/d0cad31feab6676471f18793c8feff24b2434ab0/toolkit/components/extensions/parent/ext-downloads.js#931

That issue was hit before in bug 1623704, which relaxed the original implementation (strictly expecting a Firefox browser window) to the current one (expecting any platform-specific "browser" window).

This logic does not only fail in xpcshell tests, it would also fail if Firefox is running without a browser window.

I looked into the implementation, and the window (specifically its browsingContext) usage is as follows:

• to look up the canOpenModalPicker flag. Note that this flag is always true for chrome windows.
• to look up the global to associate DOM File instances when used in the file picker. This is not relevant for the usage in the downloads API.
• to run some content analysis checks on Windows that are not relevant to chrome-privileged file pickers.
• to check if the caller is in private browsing mode in nsFilePicker::IsPrivacyModeEnabled().
  Since ext-downloads.js uses an arbitrary browser window, this may not be very effective

None of these uses depend on the input being a browser window. I think that we can safely fall back, or even only use, the (chrome privileged) window that owns the extension context (browser window for extension tabs/popups, or the headless window for background contexts).

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 2005963 - Do not require an active browser window in downloads.download()

A xpcshell test for this behavior will be added in bug 2005953.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/3ffaf75cf1b9
https://hg.mozilla.org/integration/autoland/rev/77d20d8968e6
Do not require an active browser window in downloads.download() r=extension-reviewers,rpl

Comment 3

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/77d20d8968e6

Comment 4

[Rob Wu [:robwu]]

This was not fixed yet, the patch was reverted in https://bugzilla.mozilla.org/show_bug.cgi?id=2005953#c8

This confusion caused be to reland the other patch without this patch, which resulted in another revert at https://bugzilla.mozilla.org/show_bug.cgi?id=2005953#c8

analysis: https://bugzilla.mozilla.org/show_bug.cgi?id=2005953#c10

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c16604dceb7c
https://hg.mozilla.org/integration/autoland/rev/aa9d1d3bb2bb
Do not require an active browser window in downloads.download() r=extension-reviewers,rpl

Comment 6

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/aa9d1d3bb2bb