1010344 - Comparing windows deeply makes no sense

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

There are SDK tests that do things like:

  assert.notDeepEqual(windowUtils.activeBrowserWindow, browserWindow);

This is somewhat nonsensical for two reasons:

1)  Treating two window objects as equal id they happen to quack the same in terms of properties even though they're different windows makes no sense.

2)  This code is relying on a Gecko bug: the .window property on those objects is not an own property when viewed via Xrays.  Once that's fixed, this code goes into an infinite loop any time the two windows have the same sets of own property names.

Fix coming up.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't do deep compares on window objects in addon SDK tests, since that's not a very useful thing to do and can go into infinite loops

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 8422548 [details] [diff] [review]
Don't do deep compares on window objects in addon SDK tests, since that's not a very useful thing to do and can go into infinite loops

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

Yes, this is dumb

Comment 3

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d92602ec91ca0c3c87517d72ee84b84a17dbcbb6
Bug 1010344.  Don't do deep compares on window objects in addon SDK tests, since that's not a very useful thing to do and can go into infinite loops.  r=mossop

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Backed out because something in the push was causing mochitest crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b99b42f1337

https://tbpl.mozilla.org/php/getParsedLog.php?id=39747566&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=39747507&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=39747838&tree=Mozilla-Inbound

Comment 5

[Boris Zbarsky [:bzbarsky]]

I was going to reland this (since the crashes were from bug 1004458) but looks like it landed with the SDK uplift (and _boy_ did that confuse blame).

Comment 6

[Dave Townsend [:mossop]]

Sorry about that, I saw it went in and we were doing an uplift so I made sure it got included in our repo

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's not an issue, apart from blame being very confused.