1516899 - this.properties is undefined in nsUpdateService,js * enumerate()

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P3)

Description

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

The original implementation of this wasn't correct in the first place and the changes from bug 1486249 made it so it throws this exception. The enumerator isn't used which is why the bogus implementation didn't cause any problems and is why the exception wasn't caught. I have a patch to make it match nsIPropertyBag and I'll also add a test.

Comment 1

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

Attachment: client patch rev1

Comment 2

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

Attachment: patch rev1

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

Comment 3

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

Comment on attachment 9033707 [details] [diff] [review]
patch rev1

Note: it might be a good thing and wouldn't hurt to prevent undefined values in UpdatePatch as well

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2d4be8d8c8
this.properties is undefined in nsUpdateService,js * enumerate(). r=mhowell

Comment 5

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9f2d4be8d8c8

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 9033707 [details] [diff] [review]
patch rev1

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

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1282,5 @@
>      return this.enumerate();
>    },
>  
>    * enumerate() {
> +    for (let propName in this._properties) {

Not that this is a new problem, but this should really probably be something like `for (let [name, value] of Object.entries(this._properties))` or at the very least `for (let propName of Object.keys(this._properties))`. It would be nice to fix it as long as you're refactoring this anyway.

@@ +1288,5 @@
> +        // The nsIPropertyBag enumerator returns a nsISimpleEnumerator whose
> +        // elements are nsIProperty objects.
> +        yield { name: propName,
> +                value: this._properties[propName].data,
> +                QueryInterface: ChromeUtils.generateQI([Ci.nsIProperty])};

You really don't want to generate a new QueryInterface method for every iteration. Please put the `generateQI` calls outside the loops, and reuse the generated method for all results.

@@ +1293,2 @@
>        }
>      }

If it comes to it, though, it might make more sense to just construct an nsIArray and return `array.enumerate(Ci.nsIProperty)`. The enumerators for native property bags return entries that are pre-queried to `nsIProperty` for JS callers. This one won't, unless you do something like:

  let ip = Cc["@mozilla.org/supports-interface-pointer;1"].createInstance(Ci.nsISupportsInterfacePointer);
  ...

      ip.data = { name, value: value.data, QueryInterface };
      yield ip.data.QueryInterface(Ci.nsIProperty);

which would probably be fine, too.

@@ +1612,5 @@
> +        // The nsIPropertyBag enumerator returns a nsISimpleEnumerator whose
> +        // elements are nsIProperty objects.
> +        yield { name: propName,
> +                value: this._properties[propName].data,
> +                QueryInterface: ChromeUtils.generateQI([Ci.nsIProperty])};

Same comments as above.

::: toolkit/mozapps/update/tests/unit_aus_update/updateManagerXML.js
@@ +130,5 @@
> +               "the third property name" + MSG_SHOULD_EQUAL);
> +  Assert.equal(prop.value, "true",
> +               "the third property value" + MSG_SHOULD_EQUAL);
> +  Assert.ok(!e.hasMoreElements(),
> +            "the enumerator.hasMoreElements()" + MSG_SHOULD_EQUAL);

I'd lean towards replacing all of this with something like `let results = Array.from(update.enumerator); Assert.ok(results.every(prop => prop instanceof Ci.nsIProperty); Assert.equal(results.length, ...)`

We really don't want to be using nsISimpleEnumerator directly from JS anymore.

And if you fix the enumerators to return properly-queried results, you probably want to check that the results have `name` and `value` properties before you check `instanceof nsIProperty`.

Comment 7

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

Thanks Kris! I'll take care of this in a separate bug.