Closed
Bug 1318814
Opened 8 years ago
Closed 8 years ago
`ObjectUtils.deepEqual` incorrectly returns true when comparing an object without enumerable properties and a primitive
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: lina, Assigned: lina)
Details
Attachments
(1 file)
This can manifest as `ObjectUtils.deepEqual(new Date(), 123) === true`. A simple fix is to check if `Object.prototype.toString.call(a) == Object.prototype.toString.call(b)`.
Comment hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
Comment on attachment 8812398 [details] Bug 1318814 - `ObjectUtils.deepEqual` should not consider objects and primitives as equal. https://reviewboard.mozilla.org/r/94166/#review94690 ::: toolkit/modules/ObjectUtils.jsm:73 (Diff revision 1) > // 7.1 All identical values are equivalent, as determined by ===. > if (a === b) { > return true; > + } > + if (a == null || b == null) { > + return a === b; Might as well just return false here, the above condition would have matched otherwise.
Attachment #8812398 -
Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8812398 [details] Bug 1318814 - `ObjectUtils.deepEqual` should not consider objects and primitives as equal. Ha, turns out this wasn't as simple as I thought. We have a couple of in-tree tests that depended on the lax matching. Do the new test changes still look OK to you?
Attachment #8812398 -
Flags: review+ → review?(dtownsend)
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8812398 [details] Bug 1318814 - `ObjectUtils.deepEqual` should not consider objects and primitives as equal. https://reviewboard.mozilla.org/r/94166/#review95044 Having looked closer it seems that we're following an explicit spec here that you're breaking with your changes so I don't think the test changes should be necessary. Instead I think you need to make your change less invasive and instead only fail in the specific case where one object is a Date and the other isn't. That appears to be what the spec says.
Attachment #8812398 -
Flags: review?(dtownsend) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
Sure thing, I've relaxed this in the latest patch. I think the spec we're following is http://wiki.commonjs.org/wiki/Unit_Testing/1.0, which seems quite incomplete. Writing a comprehensive deep equality implementation like https://github.com/jashkenas/underscore/blob/314eb0389c741f813fb1c9513d6572196e7531e6/underscore.js#L1197-L1285 is hard! I kept the Places test changes because those were comparing date objects against microsecond numbers, which isn't right.
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8812398 [details] Bug 1318814 - `ObjectUtils.deepEqual` should not consider objects and primitives as equal. https://reviewboard.mozilla.org/r/94166/#review95056 ::: toolkit/modules/ObjectUtils.jsm:90 (Diff revision 4) > // properties (`global`, `multiline`, `lastIndex`, `ignoreCase`). > } > - if (instanceOf(a, "RegExp") && instanceOf(b, "RegExp")) { > - return a.source === b.source && > + let aIsRegExp = instanceOf(a, "RegExp"); > + let bIsRegExp = instanceOf(b, "RegExp"); > + if (aIsRegExp || bIsRegExp) { > + return aIsRegExp && bIsRegExp && Otherwise, this wouldn't be commutative. ::: toolkit/modules/ObjectUtils.jsm:99 (Diff revision 4) > a.lastIndex === b.lastIndex && > a.ignoreCase === b.ignoreCase; > // 7.4 Other pairs that do not both pass typeof value == "object", > // equivalence is determined by ==. > } > - if (typeof a != "object" && typeof b != "object") { > + if (typeof a != "object" || typeof b != "object") { I changed this to || so that we don't return true for comparsions like `deepEqual({}, "")` and `deepEqual(123, [])`, which seems like it could mask bugs.
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8812398 [details] Bug 1318814 - `ObjectUtils.deepEqual` should not consider objects and primitives as equal. https://reviewboard.mozilla.org/r/94166/#review95382 Looks good, thanks.
Attachment #8812398 -
Flags: review?(dtownsend) → review+
Comment 11•8 years ago
|
||
Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a7389b4dca63 `ObjectUtils.deepEqual` should not consider objects and primitives as equal. r=mossop
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a7389b4dca63
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•