Closed
Bug 1517150
Opened 5 years ago
Closed 5 years ago
Don't generate a new QueryInterface method for every iteration in nsUpdateService.js enumerate
Categories
(Toolkit :: Application Update, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)
References
Details
Attachments
(1 file, 6 obsolete files)
33.72 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
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`
Assignee | ||
Comment 1•5 years ago
|
||
Pushed to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=885fdaa74d416e4efa8709ecbb8bb002330872e2
Assignee | ||
Comment 2•5 years ago
|
||
Fixed an eslint error Pushed to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a231fd4d87396430b8cec5321abeebe2f16702f
Attachment #9033875 -
Attachment is obsolete: true
Assignee | ||
Comment 3•5 years ago
|
||
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)
Assignee | ||
Comment 4•5 years ago
|
||
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)
Comment 5•5 years ago
|
||
(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 6•5 years ago
|
||
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+
Assignee | ||
Comment 7•5 years ago
|
||
(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.
Assignee | ||
Updated•5 years ago
|
Attachment #9034086 -
Flags: review?(mhowell)
Comment 8•5 years ago
|
||
(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.
Assignee | ||
Comment 9•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
Pushed to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a34e6b682349b3a53c1c33da2c7aecd115ba839
Attachment #9034086 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Attachment #9034126 -
Flags: review?(ksteuber)
Comment 11•5 years ago
|
||
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+
Assignee | ||
Comment 12•5 years ago
|
||
(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!
Assignee | ||
Comment 13•5 years ago
|
||
Attachment #9034126 -
Attachment is obsolete: true
Attachment #9034238 -
Flags: review+
Comment 14•5 years 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 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bb2c3ab2874b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment 16•5 years ago
|
||
Backed out changeset bb2c3ab2874b (Bug 1517150) for causing Bug 1517718 a=backout https://hg.mozilla.org/mozilla-central/rev/ba8f60c45276cf05701e2c34b5bf39f202e1fa65
Assignee | ||
Updated•5 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 17•5 years ago
|
||
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)
Updated•5 years ago
|
Attachment #9034403 -
Flags: review?(ksteuber) → review+
Updated•5 years ago
|
Assignee | ||
Comment 18•5 years ago
|
||
Carrying forward r+ Try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b19c902ade960faa5de14f8166bb2f9b8e454b6
Attachment #9034238 -
Attachment is obsolete: true
Attachment #9034403 -
Attachment is obsolete: true
Attachment #9034595 -
Flags: review+
Comment 19•5 years 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 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ad135e7ddad2
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•