1318814 - `ObjectUtils.deepEqual` incorrectly returns true when comparing an object without enumerable properties and a primitive

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Lina Butler [:lina]]

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 1

[Lina Butler [:lina]]

Attachment: Bug 1318814 - `ObjectUtils.deepEqual` should not consider objects and primitives as equal.

Review commit: https://reviewboard.mozilla.org/r/94166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94166/

Comment 2

[Dave Townsend [:mossop]]

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.

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8812398 [details]
Bug 1318814 - `ObjectUtils.deepEqual` should not consider objects and primitives as equal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94166/diff/1-2/

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8812398 [details]
Bug 1318814 - `ObjectUtils.deepEqual` should not consider objects and primitives as equal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94166/diff/2-3/

Comment 5

[Lina Butler [:lina]]

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?

Comment 6

[Dave Townsend [:mossop]]

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.

Comment 7

[Lina Butler [:lina]]

Comment on attachment 8812398 [details]
Bug 1318814 - `ObjectUtils.deepEqual` should not consider objects and primitives as equal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94166/diff/3-4/

Comment 8

[Lina Butler [:lina]]

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.

Comment 9

[Lina Butler [:lina]]

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

[Dave Townsend [:mossop]]

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7389b4dca63
`ObjectUtils.deepEqual` should not consider objects and primitives as equal. r=mossop

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7389b4dca63