608165 - Lots of 'reference to undefined property this.mAddon.applyBackgroundUpdates' warnings when running browser_bug562797.js in a debug build

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

With a windows debug build the following two lines are printed to the console around 200 times each when running browser_bug562797.js
JavaScript strict warning: chrome://mozapps/content/extensions/extensions.xml, line 1042: reference to undefined property this.mAddon.applyBackgroundUpdates
JavaScript strict warning: chrome://mozapps/content/extensions/extensions.xml, line 1043: reference to undefined property this.mAddon.applyBackgroundUpdates

the following two lines are printed to the console and the mochitest-browser-chrome.log around 200 times each when running browser_bug562797.js
TEST-INFO | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562797.js | Console message: [JavaScript Warning: "reference to undefined property this.mAddon.applyBackgroundUpdates" {file: "chrome://mozapps/content/extensions/extensions.xml" line: 1042}]
TEST-INFO | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562797.js | Console message: [JavaScript Warning: "reference to undefined property this.mAddon.applyBackgroundUpdates" {file: "chrome://mozapps/content/extensions/extensions.xml" line: 1043}]

I didn't check if they were printed in other tests but I would assume they are

Comment 1

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

Attachment: patch

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 486822 [details] [diff] [review]
patch

Elsewhere we treat a missing applyBackgroundUpdates property as equivalent to AUTOUPDATE_DISABLE so this should be:

if (!("applyBackgroundUpdates" in this.mAddon) || ...

Comment 3

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

The code in question with this change

if (!("applyBackgroundUpdates" in this.mAddon) ||
    (this.mAddon.applyBackgroundUpdates == AddonManager.AUTOUPDATE_DISABLE ||
     (this.mAddon.applyBackgroundUpdates == AddonManager.AUTOUPDATE_DEFAULT &&
      !AddonManager.autoUpdateDefault))) {
  var self = this;
  AddonManager.getAllInstalls(function(aInstallsList) {
    // This can return after the binding has been destroyed,
    // so try to detect that and return early
    if (!("onNewInstall" in self))
      return;
    for (let i = 0; i < aInstallsList.length; i++) {
      let install = aInstallsList[i];
      if (install.existingAddon &&
          install.existingAddon.id == self.mAddon.id &&
          install.state == AddonManager.STATE_AVAILABLE) {
        self.onNewInstall(install);
        self.onIncludeUpdateChanged();
      }
    }
  });
}

Doing it that way would not fix this bug since both sides of || would be evaluated

Another option would be

let applyBackgroundUpdates = !("applyBackgroundUpdates" in this.mAddon) ? AddonManager.AUTOUPDATE_DISABLE : this.mAddon.applyBackgroundUpdates;

if (applyBackgroundUpdates == AddonManager.AUTOUPDATE_DISABLE ||
    (applyBackgroundUpdates == AddonManager.AUTOUPDATE_DEFAULT &&
    !AddonManager.autoUpdateDefault)) {
  var self = this;
  AddonManager.getAllInstalls(function(aInstallsList) {
    // This can return after the binding has been destroyed,
    // so try to detect that and return early
    if (!("onNewInstall" in self))
      return;
    for (let i = 0; i < aInstallsList.length; i++) {
      let install = aInstallsList[i];
      if (install.existingAddon &&
          install.existingAddon.id == self.mAddon.id &&
          install.state == AddonManager.STATE_AVAILABLE) {
        self.onNewInstall(install);
        self.onIncludeUpdateChanged();
      }
    }
  });
}

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> The code in question with this change
> 
> if (!("applyBackgroundUpdates" in this.mAddon) ||
>     (this.mAddon.applyBackgroundUpdates == AddonManager.AUTOUPDATE_DISABLE ||
>      (this.mAddon.applyBackgroundUpdates == AddonManager.AUTOUPDATE_DEFAULT &&
>       !AddonManager.autoUpdateDefault))) {
>   var self = this;
>   AddonManager.getAllInstalls(function(aInstallsList) {
>     // This can return after the binding has been destroyed,
>     // so try to detect that and return early
>     if (!("onNewInstall" in self))
>       return;
>     for (let i = 0; i < aInstallsList.length; i++) {
>       let install = aInstallsList[i];
>       if (install.existingAddon &&
>           install.existingAddon.id == self.mAddon.id &&
>           install.state == AddonManager.STATE_AVAILABLE) {
>         self.onNewInstall(install);
>         self.onIncludeUpdateChanged();
>       }
>     }
>   });
> }
> 
> Doing it that way would not fix this bug since both sides of || would be
> evaluated

I don't think so, the first half would be true if there was no property and so it'd go straight into the if block then, otherwise it will test the property as expected.

Comment 5

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

Attachment: patch

Each time I work til the wee hours and then make a stupid comment after waking up I tell myself that next time I should wait until after I've had a cup of coffee before making a comment. The trouble is that I'm half asleep and forgot my earlier advice. ;)

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 486959 [details] [diff] [review]
patch

>+          if (!("applyBackgroundUpdates" in this.mAddon) &&

I think you mean || ;)

Comment 7

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

Attachment: patch

bah... need to wakeup!

Comment 8

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

I verified that the tests work with the latest patch without the debug spew.

btw: it also passed with the other two patches.

Comment 9

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

stops commenting until after coffee

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 486962 [details] [diff] [review]
patch

Explicit approval for b7 approved!

Comment 11

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

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/87e6cf44d406

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed by check-in.