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

VERIFIED FIXED in mozilla2.0b7

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
mozilla2.0b7
x86
Windows 7
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
Created attachment 486822 [details] [diff] [review]
patch
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #486822 - Flags: review?(dtownsend)
Summary: Losts of reference to undefined property this.mAddon.applyBackgroundUpdates warnings when running browser_bug562797.js in a debug build → Losts of 'reference to undefined property this.mAddon.applyBackgroundUpdates' warnings when running browser_bug562797.js in a debug build
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) || ...
Attachment #486822 - Flags: review?(dtownsend) → review-
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();
      }
    }
  });
}
(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.
Summary: Losts of 'reference to undefined property this.mAddon.applyBackgroundUpdates' warnings when running browser_bug562797.js in a debug build → Lots of 'reference to undefined property this.mAddon.applyBackgroundUpdates' warnings when running browser_bug562797.js in a debug build
Created attachment 486959 [details] [diff] [review]
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. ;)
Attachment #486822 - Attachment is obsolete: true
Attachment #486959 - Flags: review?(dtownsend)
Comment on attachment 486959 [details] [diff] [review]
patch

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

I think you mean || ;)
Attachment #486959 - Flags: review?(dtownsend) → review+
Created attachment 486962 [details] [diff] [review]
patch

bah... need to wakeup!
Attachment #486959 - Attachment is obsolete: true
Attachment #486962 - Flags: review+
I verified that the tests work with the latest patch without the debug spew.

btw: it also passed with the other two patches.
stops commenting until after coffee
Comment on attachment 486962 [details] [diff] [review]
patch

Explicit approval for b7 approved!
Attachment #486962 - Flags: approval2.0+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/87e6cf44d406
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
Verified fixed by check-in.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.