1149334 - Remove preprocessing of nsUpdateService.js and use AppConstants.jsm

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

There is AppConstants.jsm and the caching mechanisms for components are much better (much of it was added to reduce file size so loading nsUpdateService.js on startup didn't suck on WinCE). It is no longer needed and finding line numbers with preprocessing is a pita.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch - AppConstants.jsm additions

Gavin, to replace preprocessing in nsUpdateService.js I need this added to AppConstants.jsm.

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch - main

Comment 3

[Robert Strong (they/them - no direct email)]

Pushed to try
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=384297503f45

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 8585804 [details] [diff] [review]
patch - main

utlConstruction.js has a failure but otherwise it looks good. I'll resubmit after I figure out what is going on for that test.

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: patch - main rev2

simple fix

Comment 6

[Robert Strong (they/them - no direct email)]

New try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a46a640660fb

Comment 7

[Stephen A Pohl [:spohl]]

Comment on attachment 8585841 [details] [diff] [review]
patch - main rev2

B2G seems to run into failures as well. Clearing r? request for now.

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: patch - main rev3

Should fix the B2G issue
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ccb4b2d2e0d

Comment 9

[Robert Strong (they/them - no direct email)]

Try run didn't start likely due to the same build issues that closed the trees.

New try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29968a9bd5c7

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8585803 [details] [diff] [review]
patch - AppConstants.jsm additions

Is MOZ_SHARK still useful? I didn't think Shark as a tool existed still. If so we should probably remove the code rather than adding it here.

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: patch - AppConstants.jsm additions rev2

We no longer create updates for shark so removing. I asked ted whether it is ever still used and he didn't know. I'll ask around and file a new bug to remove it if it isn't used.

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: patch -main rev4

Removed MOZ_SHARK usage

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 8586396 [details] [diff] [review]
patch -main rev4

Still a bug on the b2g emulator... I wish it was dirt simple to run those locally.

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: patch - main rev5

New try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51c12fb9a1a4

Comment 15

[Robert Strong (they/them - no direct email)]

Dave, it appears that on B2G the AppConstants.jsm is available in debug builds and it is not in opt builds.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51c12fb9a1a4

For example with an opt build
18:15:15     INFO -  PROCESS | toolkit/mozapps/update/tests/unit_aus_update/canCheckForAndCanApplyUpdates.js | *** AUS:SVC Creating UpdateService
18:15:15     INFO -  PROCESS | toolkit/mozapps/update/tests/unit_aus_update/canCheckForAndCanApplyUpdates.js | JavaScript error: jar:file:///system/b2g/omni.ja!/components/nsUpdateService.js, line 1949: TypeError: AppConstants is undefined

and with a debug build
18:34:45     INFO -  TEST-START | toolkit/mozapps/update/tests/unit_aus_update/canCheckForAndCanApplyUpdates.js
18:34:56     INFO -  TEST-PASS | toolkit/mozapps/update/tests/unit_aus_update/canCheckForAndCanApplyUpdates.js | took 11349ms

I also tried loading AppConstants.jsm in the xpcshell test itself with the same result.

Can you help out with this?

Comment 16

[Robert Strong (they/them - no direct email)]

Try push with
  let ac = (Cu.import("resource://gre/modules/AppConstants.jsm", {}));
  dump("AppConstants source\n");
  dump(JSON.stringify(ac) + "\n");

to try to find out what is being imported for opt builds. I also intentionally failed one test to compare with dbg builds.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad03a77dd276

Comment 17

[Dave Hylands [:dhylands]]

I can reproduce the problem, but I don't have an explanation yet.

I verified that modules/AppConstants.jsm is present in the omni.ja file for both cases (and the 2 files are identical).

I tried using the debug omni.ja with the release version of b2g and I still got the error. So I think that means its got something to do with the C++ code.

I tried changing:

> let AppConstants = Object.freeze({

to be:

> this.AppConstants = Object.freeze({

seems to fix things (I looked at FileUtils.jsm to see how it was implemented.

I have no idea why this would only affect B2G non-debug.

Comment 18

[Robert Strong (they/them - no direct email)]

(In reply to Dave Hylands [:dhylands] from comment #17)
> I can reproduce the problem, but I don't have an explanation yet.
> 
> I verified that modules/AppConstants.jsm is present in the omni.ja file for
> both cases (and the 2 files are identical).
> 
> I tried using the debug omni.ja with the release version of b2g and I still
> got the error. So I think that means its got something to do with the C++
> code.
The stringify try build had the following

opt:
{"EXPORTED_SYMBOLS":["AppConstants"]}

dbg:
{"AppConstants":{"NIGHTLY_BUILD":true,"RELEASE_BUILD":false,"ACCESSIBILITY":true,"MOZILLA_OFFICIAL":false,"MOZ_OFFICIAL_BRANDING":false,"MOZ_SERVICES_HEALTHREPORT":false,"MOZ_DEVICES":false,"MOZ_SAFE_BROWSING":true,"MOZ_TELEMETRY_REPORTING":false,"MOZ_WEBRTC":true,"platform":"gonk","MOZ_CRASHREPORTER":true,"MOZ_MAINTENANCE_SERVICE":false,"E10S_TESTING_ONLY":true,"MOZ_APP_VERSION":"40.0a1","ANDROID_PACKAGE_NAME":"org.mozilla.b2g_cltbld"},"EXPORTED_SYMBOLS":["AppConstants"]}

> I tried changing:
> 
> > let AppConstants = Object.freeze({
> 
> to be:
> 
> > this.AppConstants = Object.freeze({
> 
> seems to fix things (I looked at FileUtils.jsm to see how it was implemented.
> 
> I have no idea why this would only affect B2G non-debug.
The try log made me suspicious about the let as well so I pushed that change to try a short while ago.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cc1977f8343

Thanks Dave!

Comment 19

[Robert Strong (they/them - no direct email)]

Filed bug 1150312 to find out whether MOZ_SHARK is still used (I asked glandium and ted but neither of them knew).

Comment 20

[Robert Strong (they/them - no direct email)]

Attachment: patch - AppConstants.jsm additions rev3

Gavin, using let to declare AppConstants is what caused the B2G failures. The GC on B2G is rather aggressive iirc and tbh let at this scope never made sense to me.

Comment 21

[Robert Strong (they/them - no direct email)]

Attachment: patch - main rev6

Try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee5da0bac88d

Comment 22

[Robert Strong (they/them - no direct email)]

Comment on attachment 8587098 [details] [diff] [review]
patch - main rev6

Try push looked good though I canceled a couple of them since at that time the Win 8 x64 queue was huge. One more try run for good measure
https://treeherder.mozilla.org/#/jobs?repo=try&revision=94350ec4a945

Comment 23

[Stephen A Pohl [:spohl]]

Comment on attachment 8587098 [details] [diff] [review]
patch - main rev6

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

nsUpdateService.js could benefit from some general |var| vs |let| cleanup which doesn't need to occur in this bug. I've tried to only point out the places that were in code that got changed by this patch, or if the variable is mostly used in code that got changed.

Also, GMPInstallManager.jsm has code that was copied directly from nsUpdateService.js. I'll go ahead and file a bug to update the GMPInstallManager.jsm file with the new code here.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +501,5 @@
>    }
>  
>    if (!useService) {
>      try {
>        var updateTestFile = getUpdateFile([FILE_PERMS_TEST]);

nit: let updateTestFile

@@ +510,1 @@
>          var appDirTestFile = getAppBaseDir();

nit: let appDirTestFile

@@ +530,5 @@
> +       * Note: this does note attempt to handle the case where UAC is turned on
> +       * and the installation directory is in a restricted location that
> +       * requires admin privileges to update other than Program Files.
> +       */
> +        var userCanElevate = false;

nit: let userCanElevate

@@ +536,5 @@
> +        if (parseFloat(windowsVersion) >= 6) {
> +          try {
> +            // KEY_UPDROOT will fail and throw an exception if
> +            // appDir is not under the Program Files, so we rely on that
> +            var dir = Services.dirsvc.get(KEY_UPDROOT, Ci.nsIFile);

nit: let dir

@@ +572,5 @@
> +         *    elevated again
> +         */
> +        if (!userCanElevate) {
> +          // if we're unable to create the test file this will throw an exception.
> +          var appDirTestFile = getAppBaseDir();

nit: let appDirTestFile

@@ +1001,3 @@
>  
>  /**
>   * Releases any SDCard mount lock that we might have.

Should we add a comment here stating that we only expect gonk to call this function?

@@ +1036,5 @@
>   *          is installed and enabled.
>   */
>  function isServiceInstalled() {
> +  if (AppConstants.MOZ_MAINTENANCE_SERVICE) {
> +    if (AppConstants.platform == "win") {

Could we combine these two |if|s?

@@ +1947,5 @@
>    Services.obs.addObserver(this, "xpcom-shutdown", false);
>    Services.prefs.addObserver(PREF_APP_UPDATE_LOG, this, false);
> +  let ac = (Cu.import("resource://gre/modules/AppConstants.jsm", {}));
> +  dump("AppConstants source\n");
> +  dump(JSON.stringify(ac) + "\n");

Should this have been removed?

@@ +3389,5 @@
> +        prompter.showUpdateDownloaded(update, true);
> +      } else {
> +        releaseSDCardMountLock();
> +      }
> +      return;

Can you just confirm that the addition of this return here is intentional?

@@ +3478,5 @@
> +      url = url.replace(/%PRODUCT_MODEL%/g,
> +                        sysLibs.libcutils.property_get("ro.product.model"));
> +      url = url.replace(/%PRODUCT_DEVICE%/g,
> +                        sysLibs.libcutils.property_get("ro.product.device"));
> +      url = url.replace(/%B2G_VERSION%/g, getPref("getCharPref", PREF_APP_B2G_VERSION, null));

nit: Looks like this line might be > 80 characters.

@@ +3904,4 @@
>        }
>  
> +      // Something went wrong when we tried to apply the previous patch.
> +      // Try the complete patch next time.

Seems like the "Try the complete patch next time."-part of this comment would be more appropriate after the |if| block.

@@ +3999,5 @@
>        return readStatusFile(updateDir);
>      }
>      this.isCompleteUpdate = this._patch.type == "complete";
>  
>      var patchFile = null;

nit: let patchFile

@@ +4415,5 @@
>          }
>  
> +        if (AppConstants.platform == "gonk") {
> +          // We always forward errors in B2G, since Gaia controls the update UI
> +          var prompter = Cc["@mozilla.org/updates/update-prompt;1"].

nit: let prompter

Comment 24

[Robert Strong (they/them - no direct email)]

Done

(In reply to Stephen Pohl [:spohl] from comment #23)
> Comment on attachment 8587098 [details] [diff] [review]
>...
> @@ +3389,5 @@
> > +        prompter.showUpdateDownloaded(update, true);
> > +      } else {
> > +        releaseSDCardMountLock();
> > +      }
> > +      return;
> 
> Can you just confirm that the addition of this return here is intentional?
> 
The old code is as follows and the return is there instead of doing an else statement

#ifdef MOZ_WIDGET_GONK
    if (update.state == STATE_APPLIED) {
      // Notify the user that an update has been staged and is ready for
      // installation (i.e. that they should restart the application). We do
      // not notify on failed update attempts.
      var prompter = Cc["@mozilla.org/updates/update-prompt;1"].
                     createInstance(Ci.nsIUpdatePrompt);
      prompter.showUpdateDownloaded(update, true);
    } else {
      releaseSDCardMountLock();
    }
#else
    // Only prompt when the UI isn't already open.
    let windowType = getPref("getCharPref", PREF_APP_UPDATE_ALTWINDOWTYPE, null);
    if (Services.wm.getMostRecentWindow(UPDATE_WINDOW_NAME) ||
        windowType && Services.wm.getMostRecentWindow(windowType)) {
      return;
    }

    if (update.state == STATE_APPLIED || update.state == STATE_APPLIED_SVC ||
        update.state == STATE_PENDING || update.state == STATE_PENDING_SVC) {
      // Notify the user that an update has been staged and is ready for
      // installation (i.e. that they should restart the application).
      var prompter = Cc["@mozilla.org/updates/update-prompt;1"].
                     createInstance(Ci.nsIUpdatePrompt);
      prompter.showUpdateDownloaded(update, true);
    }
#endif
  },

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8587097 [details] [diff] [review]
patch - AppConstants.jsm additions rev3

Ah, the "this." thing is not related to GC, it's because of bug 798491 - B2G has different behavior for JSMs (runs them all in the same compartment) which necessitates changes to how modules are defined. We just failed to do that properly when we landed AppConstants.

Comment 26

[Robert Strong (they/them - no direct email)]

Attachment: patch - main comments addressed

Comment 27

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/6156c8d856a2
https://hg.mozilla.org/integration/fx-team/rev/3ca42bb579e5

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6156c8d856a2
https://hg.mozilla.org/mozilla-central/rev/3ca42bb579e5