Don't generate a new QueryInterface method for every iteration in nsUpdateService.js enumerate

RESOLVED FIXED in Firefox 66

Status

()

defect
P3
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

59 Branch
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

This is to address comments in bug 1516899 comment #6

::: 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`
Depends on: 1516899
Priority: -- → P3
Comment on attachment 9033876 [details] [diff] [review]
patch rev2

Hi Kris, as noted in the patch's comments I ended up going with nsISupportsInterfacePointer since using Array.from wouldn't have the QueryInterface info for nsIProperty without it even though the objects returned by getNext() do have it. Also, calling QueryInterface(Ci.nsIProperty) on nsISupportsInterfacePointer data didn't return a pre-queried nsIProperty object to the caller. I recall this being an issue years ago when using JavaScript components and I'm fine with it as is but if there is a trick you know that makes that work I'll add it.
Attachment #9033876 - Flags: review?(kmaglione+bmo)
Posted patch patch 1 (obsolete) — Splinter Review
Kris, I split out the changes you requested into this patch to simplify getting your feedback and will submit a second patch for the unrelated changes.
Attachment #9033876 - Attachment is obsolete: true
Attachment #9033876 - Flags: review?(kmaglione+bmo)
Attachment #9034086 - Flags: review?(mhowell)
Attachment #9034086 - Flags: feedback?(kmaglione+bmo)
(In reply to Robert Strong (Robert he/him) [:rstrong] (use needinfo to contact me) from comment #3)
> Also, calling
> QueryInterface(Ci.nsIProperty) on nsISupportsInterfacePointer data didn't
> return a pre-queried nsIProperty object to the caller. I recall this being
> an issue years ago when using JavaScript components and I'm fine with it as
> is but if there is a trick you know that makes that work I'll add it.

Hm. This is probably because the mochitest runs in a different compartment from the JSM scope. Creating an nsIArray and calling `array.enumerate(Ci.nsIProperty)` wouldn't have this problem, but it should also mostly go away after bug 1514210, so it may not be worth changing.
Comment on attachment 9034086 [details] [diff] [review]
patch 1

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

This mostly looks good, but if we're going to change the format of the `_properties` object anyway, we should probably just make it a Map.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1241,5 @@
>      patch.setAttribute("type", this.type);
>      patch.setAttribute("URL", this.URL);
>  
> +    for (let [name, value] of Object.entries(this._properties)) {
> +      patch.setAttribute(name, value);

I think this needs to be `if (value.present) { patch.setAttribute(name, value.data); }`

@@ +1264,5 @@
>     * See nsIWritablePropertyBag.idl
>     */
>    deleteProperty: function UpdatePatch_deleteProperty(name) {
> +    if (name in this._properties) {
> +      delete this._properties[name];

As a general rule, we should try to avoid deleting object properties as much as possible. That immediately puts the objects into dictionary mode, which takes away most of their benefits, and is usually a sign we should be using a `Map` instead.

@@ +1292,5 @@
> +      // elements are nsIProperty objects. There is no point in calling
> +      // QueryInterface for nsIProperty on the object since this doesn't return
> +      // an object that is already queried to nsIProperty to the caller.
> +      ip.data = {name, value, QueryInterface: qi};
> +      yield ip.data;

Please use `ip.data.QueryInterface(Ci.nsIProperty)`. It won't have an affect on the tests, for the moment, but it should still help same-compartment callers, which will be pretty much everything after bug 1514210 lands.
Attachment #9034086 - Flags: feedback?(kmaglione+bmo) → feedback+
(In reply to Kris Maglione [:kmag] from comment #5)
> (In reply to Robert Strong (Robert he/him) [:rstrong] (use needinfo to
> contact me) from comment #3)
> > Also, calling
> > QueryInterface(Ci.nsIProperty) on nsISupportsInterfacePointer data didn't
> > return a pre-queried nsIProperty object to the caller. I recall this being
> > an issue years ago when using JavaScript components and I'm fine with it as
> > is but if there is a trick you know that makes that work I'll add it.
> 
> Hm. This is probably because the mochitest runs in a different compartment
> from the JSM scope. Creating an nsIArray and calling
> `array.enumerate(Ci.nsIProperty)` wouldn't have this problem, but it should
> also mostly go away after bug 1514210, so it may not be worth changing.

This is a js xpcom component and not a jsm. I saw this years ago with other js xpcom components well before we even had jsm's. Whatever the case, there have never been any consumers of these enumerators.
(In reply to Robert Strong (Robert he/him) [:rstrong] (use needinfo to contact me) from comment #7)
> This is a js xpcom component and not a jsm. I saw this years ago with other
> js xpcom components well before we even had jsm's. Whatever the case, there
> have never been any consumers of these enumerators.

It doesn't matter. They're basically the same thing. They all load in the same global, in the same type of environment.

I'm mainly just trying to avoid footguns if we add consumers in the future.
Not a problem. I mention that because this was around 2005 (did we have compartments then?) and iirc I was told this had to do with going from the js xpcom component to c++ and then to js.
Comment on attachment 9034126 [details] [diff] [review]
patch

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

Looks good except for one or two little things.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1212,5 @@
> +        break;
> +      case "errorCode":
> +        if (attr.value) {
> +          let val = parseInt(attr.value);
> +          if (val) {

This will evaluate to `false` if val is 0. But val shouldn't be 0, so whether you change this is up to you.

@@ +1216,5 @@
> +          if (val) {
> +            this.errorCode = val;
> +          }
> +        }
> +        this[attr.name] = attr.value;

If an integer value is set above, this will overwrite it.
Attachment #9034126 - Flags: review?(ksteuber) → review+
(In reply to Kirk Steuber (he/him) [:bytesized] from comment #11)
> Comment on attachment 9034126 [details] [diff] [review]
> patch
> 
> Review of attachment 9034126 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good except for one or two little things.
> 
> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +1212,5 @@
> > +        break;
> > +      case "errorCode":
> > +        if (attr.value) {
> > +          let val = parseInt(attr.value);
> > +          if (val) {
> 
> This will evaluate to `false` if val is 0. But val shouldn't be 0, so
> whether you change this is up to you.
That is intentional. At the beginning of the constructor there is this.errorCode = 0; I should add a comment to make this clearer.

> @@ +1216,5 @@
> > +          if (val) {
> > +            this.errorCode = val;
> > +          }
> > +        }
> > +        this[attr.name] = attr.value;
> 
> If an integer value is set above, this will overwrite it.
Good catch and thanks!
Posted patch patch as landed (obsolete) — Splinter Review
Attachment #9034126 - Attachment is obsolete: true
Attachment #9034238 - Flags: review+

Comment 14

5 months ago
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2c3ab2874b
Don't generate a new QueryInterface method for every iteration in nsUpdateService.js enumerate and other fixes. r=bytesized

Comment 15

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bb2c3ab2874b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch Patch followup (obsolete) — Splinter Review
The patch didn't handle text nodes in the update xml. This fixes that as well as updates the tests so they will catch this if it happens again.
Attachment #9034403 - Flags: review?(ksteuber)
Attachment #9034403 - Flags: review?(ksteuber) → review+

Comment 19

5 months ago
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad135e7ddad2
Don't generate a new QueryInterface method for every iteration in nsUpdateService.js enumerate and other fixes. r=bytesized

Comment 20

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ad135e7ddad2
Status: REOPENED → RESOLVED
Last Resolved: 5 months ago5 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.