928349 - Add a build-time setting to use only the JavaScript API for downloads, and enable it in Firefox for Desktop

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

Currently, in Firefox for Desktop, there are about:config preferences that
allow disabling the asynchronous JavaScript API for downloads, as well as
the Downloads Panel itself.

These preferences were introduced to allow for a transitory period while
the new features were developed, and they remained available for some time
despite the code they enabled was unmaintained.

Due to the fact that the old code is unmaintained, toggling those preferences
puts the browser in a condition that incurs in security issues (bug 916126),
stability issues (bug 926146), and performance issues in the form of
main-thread I/O, in particular starting from Firefox 26.

We should introduce a build-time setting that replaces the preferences in
Firefox for Desktop, so that in Firefox 26 we can enable the new code only.
The build-time setting will stay disabled in other Mozilla products until
they completely migrate to the new JavaScript API.

Comment 2

[:Paolo Amadini]

Attachment: The patch

This patch reduces the maintenance surface by introducing the MOZ_JSDOWNLOADS
build-time setting, enabled in the "confvars.sh" file of Firefox for Desktop,
replacing the uses of the "useToolkitUI" and "useJSTransfer" preferences.

Toolkit will still support both the build-time setting and the preferences for
a short time, so that other products can use the preferences for migration
without requiring a build-time setting at first. I've used a "feature
detection" approach using the "activeDownloadCount" property for simplicity.

In the front-end code, I've made the minimum amount of changes, because this
will make the Aurora uplift for Firefox 26 much easier. For example, I've
kept the "defineLazyGetter" in place even if it is unnecessary. The dead code
will be removed in bug 899110, that may be fixed in Firefox 27 or Firefox 28.

Some code in "nsBrowserGlue.js" is not needed for migration anymore. I've
removed a reference to the unused "browser.library.useNewDownloadsView".

The check in "DownloadTaskbarProgress.jsm" is not needed anymore, because
the module is only used in conjunction with the old Download Manager.
I've aligned "test_taskbarprogress_service.xul" to use the common feature
detection code as well.

Comment 3

[Neil Deakin]

Comment on attachment 819025 [details] [diff] [review]
The patch

>@@ -572,20 +569,17 @@ XPCOMUtils.defineLazyGetter(DownloadsCom
> /**
>  * Returns true if we should hook the panel to the JavaScript API for downloads
>  * instead of the nsIDownloadManager back-end.  In order for the logic to work
>  * properly, this value never changes during the execution of the application,
>  * even if the underlying preference value has changed.  A restart is required
>  * for the change to take effect.
>  */
> XPCOMUtils.defineLazyGetter(DownloadsCommon, "useJSTransfer", function () {
>-  try {
>-    return Services.prefs.getBoolPref("browser.download.useJSTransfer");
>-  } catch (ex) { }
>-  return false;
>+  return true;
> });

Change the comment here as it now always returns true.


>diff --git a/browser/components/downloads/src/DownloadsStartup.js b/browser/components/downloads/src/DownloadsStartup.js
>         // If the integration preference is enabled, override Toolkit's
>         // nsITransfer implementation with the one from the JavaScript API for
>         // downloads.  This should be used only by developers while testing new
>         // code that uses the JavaScript API, and will eventually be removed
>         // when nsIDownloadManager will not be available anymore (bug 851471).

Change this comment as well.


>+#ifndef MOZ_JSDOWNLOADS
> BROWSER_CHROME_MANIFESTS += ['browser.ini']
>-
>+#endif

I assume the tests are no longer relevant?


>-    // Taskbar progress is disabled until this component is updated to use the
>-    // asynchronous JavaScript API for downloads.
>-    try {
>-      if (Services.prefs.getBoolPref("browser.download.useJSTransfer")) {
>-        DownloadTaskbarProgressUpdater = null;
>-        return;
>-      }
>-    } catch (ex) { }
>-

I assume this has been updated to use the new api. I don't see where that happened.

Comment 4

[:Paolo Amadini]

Attachment: Updated patch

(In reply to Neil Deakin from comment #3)
> >+#ifndef MOZ_JSDOWNLOADS
> > BROWSER_CHROME_MANIFESTS += ['browser.ini']
> >-
> >+#endif
> 
> I assume the tests are no longer relevant?

Yes, they were already disabled by the preference.

> >-    // Taskbar progress is disabled until this component is updated to use the
> >-    // asynchronous JavaScript API for downloads.
> >-    try {
> >-      if (Services.prefs.getBoolPref("browser.download.useJSTransfer")) {
> >-        DownloadTaskbarProgressUpdater = null;
> >-        return;
> >-      }
> >-    } catch (ex) { }
> >-
> 
> I assume this has been updated to use the new api. I don't see where that
> happened.

The front-end code currently imports DownloadTaskbar instead of DownloadTaskbarProgress. We could have removed this piece of code before this patch as well.

Comment 5

[:Paolo Amadini]

I started a tryserver build to check if the tests continue to work correctly on products and platforms where the build time setting is disabled:

https://tbpl.mozilla.org/?tree=Try&rev=c2a58b9403e9

Comment 6

[:Paolo Amadini]

We should uplift this change in order to reduce the maintenance and support surface of Firefox 26 installations, and prevent the issues described in comment 0.

Comment 7

[:Paolo Amadini]

The ForgetAboutSite tests were missing from the previous patch. I think we can disable them on Desktop for now (we still run them on non-Desktop platforms), and I've filed bug 928972 to reimplement them with the new API.

Comment 8

[:Paolo Amadini]

Attachment: Patch with MOZ_JSDOWNLOADS enabled on Metro

This is updated to take into account the tests and fixes the moz.build changes.
Asking review for the moz.build changes now that they pass on try.

I need to disable MOZ_JSDOWNLOADS on Metro builds. How can I do that?

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 819768 [details] [diff] [review]
Patch with MOZ_JSDOWNLOADS enabled on Metro

Review of attachment 819768 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +8331,5 @@
>  
> +AC_SUBST(MOZ_JSDOWNLOADS)
> +if test -n "$MOZ_JSDOWNLOADS"; then
> +  AC_DEFINE(MOZ_JSDOWNLOADS)
> +fi

If the JS Downloads API is the future, it seems to me that we should have it enabled by default and have apps opt out of the legacy implementation. Is this a reasonable request?

Comment 10

[:Paolo Amadini]

(In reply to Gregory Szorc [:gps] from comment #9)
> If the JS Downloads API is the future, it seems to me that we should have it
> enabled by default and have apps opt out of the legacy implementation. Is
> this a reasonable request?

The reality is that, at present, everything except Desktop has opted to stay on the old API, or has not allocated the resources for migration yet. This is the safest approach and doesn't need comm-central follow-ups for now.

We'll definitely switch to the other way around as part of the work to reach bug 851471.

Comment 11

[Matt Brubeck (:mbrubeck)]

(In reply to :Paolo Amadini from comment #8)
> I need to disable MOZ_JSDOWNLOADS on Metro builds. How can I do that?

We don't build Metro separately.  Our Windows build includes both the desktop and Metro front-ends, which share a single build of toolkit and gecko.  So we can't use separate build flags for Metro, and we can't stop building the old download manager code on Windows until Metro stops using it (bug 906042).

Comment 12

[:Paolo Amadini]

What I think I can do here is to modify nsDownloadManager.cpp and ForgetAboutSite.jsm to use a different preference instead of the build-time setting, but keep the build-time setting in place for the rest, together with the desktop front-end modifications and the test changes.

Comment 13

[:Paolo Amadini]

Attachment: Metro-compatible patch

I still need to see if this patch compiles on platforms other than Linux, but
this approach should work without requiring additional preferences.

I've disabled one additional failing test about Content-Encoding, that we have
already reimplemented in the JavaScript API. I've not removed it entirely, to
keep the patch simple and in case the "test-ipc" folder is useful for future
Electrolysis work.

Comment 14

[:Paolo Amadini]

The tryserver build passes on all platforms:

https://tbpl.mozilla.org/?tree=Try&rev=4ae4e912d3f2

Comment 15

[Neil Deakin]

Comment on attachment 820282 [details] [diff] [review]
Metro-compatible patch

> #FIXME/bug 575918: out-of-process xpcshell is broken on OS X
> if CONFIG['OS_ARCH'] != 'Darwin':
>-    XPCSHELL_TESTS_MANIFESTS += ['unit_ipc/xpcshell.ini']
>+    # The encoding test is already implemented in the Downloads API
>+    if not CONFIG['MOZ_JSDOWNLOADS']:
>+        XPCSHELL_TESTS_MANIFESTS += ['unit_ipc/xpcshell.ini']

Can you indicate here which test replaces this?

Comment 16

[:Paolo Amadini]

(In reply to Neil Deakin from comment #15)
> Can you indicate here which test replaces this?

Pushed to inbound with the comment change:

https://hg.mozilla.org/integration/fx-team/rev/70e1e02b917d

Comment 17

[:Paolo Amadini]

Comment on attachment 820282 [details] [diff] [review]
Metro-compatible patch

Per comment 6, we should uplift this change in order to reduce the maintenance and support surface of Firefox 26 installations, and prevent the issues described in comment 0.

A tryserver build for the Aurora branch is currently running:

https://tbpl.mozilla.org/?tree=Try&rev=af96fe536905

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70e1e02b917d

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 820282 [details] [diff] [review]
Metro-compatible patch

What can be added to (or is already in) our Test Plan for FF26 to make sure this is working correctly and as intended when we get to Beta and a larger audience?

Comment 20

[Joshua Cranmer [:jcranmer]]

Comment on attachment 820282 [details] [diff] [review]
Metro-compatible patch

Review of attachment 820282 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/test/unit/head_download_manager.js
@@ +243,5 @@
>  
>  function oldDownloadManagerDisabled() {
>    try {
> +    // This method throws an exception if the old Download Manager is disabled.
> +    Services.downloads.activeDownloadCount;

This change breaks test_old_download_files_removed.js, as it causes the download manager to be initialized at the wrong time for the test. Of course, anyone who doesn't have this disabled won't notice it, since the test won't run.

As a result, we have perma-orange on comm-central tests.

Comment 21

[Joshua Cranmer [:jcranmer]]

I'd like to see this get resolved somehow. Perhaps we could remove the test altogether, since it seems that this is talking about migrating a datafile that was obsoleted 5 years ago, if the comments in source code are correct.

Comment 22

[Joshua Cranmer [:jcranmer]]

(In reply to Joshua Cranmer [:jcranmer] from comment #21)
> I'd like to see this get resolved somehow. Perhaps we could remove the test
> altogether, since it seems that this is talking about migrating a datafile
> that was obsoleted 5 years ago, if the comments in source code are correct.

I should have waited until I investigated both current errors before commenting, since it turns out that the second error on comm-central is the same issue:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain_activeDownloads.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain_activeDownloads.js | false == true - See following stack:

Turns out that the code was manually setting up an older database schema and relying on the later initialization to migrate to the newer schema. Since, again, the check for old-versus-new causes the initialization now, it happens prior to the schema is setup, so the test doesn't see the schema changes.

Comment 23

[:Paolo Amadini]

(In reply to [e-mail] [:lsblakk] from comment #19)
> What can be added to (or is already in) our Test Plan for FF26 to make sure
> this is working correctly and as intended when we get to Beta and a larger
> audience?

Create two boolean preferences named "browser.download.useToolkitUI" and "browser.download.useJSTransfer". With any of the four possible combinations of values of those preferences, downloads should be displayed in the Downloads Panel. The browser should be restarted before each test, after changing the preferences.

Comment 24

[:Paolo Amadini]

Attachment: Remove obsolete tests currently failing on comm-central

(In reply to Joshua Cranmer [:jcranmer] from comment #22)
> TEST-UNEXPECTED-FAIL |
> /builds/slave/test/build/xpcshell/tests/toolkit/forgetaboutsite/test/unit/
> test_removeDataFromDomain_activeDownloads.js | test failed (with xpcshell
> return code: 0), see following log:

We're going to remove this test in bug 928972 anyways, we might as well do this now. I've also removed the obsolete nsIDownloadManager database tests, that as you noticed have outlived their usefulness.

Eventually, we'll remove nsIDownloadManager from Toolkit. This might mean deleting it or moving it to comm-central based on the results of bug 888915 and bug 907732.

I've started https://tbpl.mozilla.org/?tree=Try&rev=883af50b2342, can you verify if this solves all the issues on comm-central as well?

Comment 25

[:Paolo Amadini]

I did the same changes in the Aurora patch, running on tryserver again for verification:

https://tbpl.mozilla.org/?tree=Try&rev=668bcaea92bd

I think we can land this one if the tests succeed.

Comment 26

[:Paolo Amadini]

The Aurora tryserver build incorporating the test removals was green, so I went ahead and landed:

https://hg.mozilla.org/releases/mozilla-aurora/rev/651537e2968a

Comment 27

[Joshua Cranmer [:jcranmer]]

Comment on attachment 822214 [details] [diff] [review]
Remove obsolete tests currently failing on comm-central

Review of attachment 822214 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes the errors for me, thanks for your quick response!

Comment 28

[:Paolo Amadini]

Follow-up landed in fx-team, thanks for the quick verification!

https://hg.mozilla.org/integration/fx-team/rev/cdcf2452599e

Comment 29

[Phil Ringnalda (:philor)]

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

Comment 30

[Ioana (away)]

Verified as fixed on Firefox 26b1 - 20131028225529 - on Windows 7, Ubuntu 13.04 and Mac OS X 10.9.

All combinations of values for the prefs mentioned in comment 23 lead to the same behavior - the downloads are shown in the panel.

Comment 31

[Ada [:adalucinet]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0

Verified as fixed on latest Aurora (Build ID: 20131105004004) with STRs from comment 23: with any of the four possible combinations of values for those 2 prefs, the downloads are shown in the panel.

Comment 32

[Patrick Storz]

*sad*

see bug 948964