Remove preprocessing of nsUpdateService.js and use AppConstants.jsm

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 8 obsolete attachments)

1.11 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
95.33 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
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.
OS: Windows 8.1 → All
Hardware: x86_64 → All
Summary: Remove preprocessing of nsUpdateService.js → Remove preprocessing of nsUpdateService.js and use AppConstants.jsm
Gavin, to replace preprocessing in nsUpdateService.js I need this added to AppConstants.jsm.
Attachment #8585803 - Flags: review?(gavin.sharp)
Posted patch patch - main (obsolete) — Splinter Review
Attachment #8585804 - Flags: review?(spohl.mozilla.bugs)
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.
Attachment #8585804 - Flags: review?(spohl.mozilla.bugs)
Posted patch patch - main rev2 (obsolete) — Splinter Review
simple fix
Attachment #8585804 - Attachment is obsolete: true
Attachment #8585841 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8585841 [details] [diff] [review]
patch - main rev2

B2G seems to run into failures as well. Clearing r? request for now.
Attachment #8585841 - Flags: review?(spohl.mozilla.bugs)
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 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.
Attachment #8585803 - Flags: review?(gavin.sharp) → review+
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.
Attachment #8585803 - Attachment is obsolete: true
Attachment #8586348 - Flags: review+
Posted patch patch -main rev4 (obsolete) — Splinter Review
Removed MOZ_SHARK usage
Attachment #8586261 - Attachment is obsolete: true
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.
Attachment #8586396 - Attachment is obsolete: true
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?
Flags: needinfo?(dhylands)
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
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.
Flags: needinfo?(dhylands)
(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!
Filed bug 1150312 to find out whether MOZ_SHARK is still used (I asked glandium and ted but neither of them knew).
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.
Attachment #8586348 - Attachment is obsolete: true
Attachment #8587097 - Flags: review?(gavin.sharp)
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
Attachment #8587098 - Flags: review?(spohl.mozilla.bugs)
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
Attachment #8587098 - Flags: review?(spohl.mozilla.bugs) → review+
Blocks: 1150688
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 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.
Attachment #8587097 - Flags: review?(gavin.sharp) → review+
You need to log in before you can comment on or make changes to this bug.