Closed Bug 954811 Opened 8 years ago Closed 8 years ago

aContactA/B errors when buddies sign in or out

Categories

(Instantbird :: Contacts window, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: florian)

References

Details

(Whiteboard: [1.2-blocking])

Attachments

(1 file)

*** Original post on bio 1377 at 2012-04-13 15:06:00 UTC ***

I occasionally get errors saying that aContactA / aContactB doesn't exist or is undefined. I don't have any STR, but it happens often enough (possibly when a merged contact signs in, or if the higher priority contacts sign out?)
*** Original post on bio 1377 at 2012-04-13 15:08:59 UTC ***

I saw this several times and each time I could notice that there were invisible contact elements in some groups (to check, count the number of contacts displayed in the group, and compare with the number that appears when closing the group).
Attached patch FixSplinter Review
*** Original post on bio 1377 as attmnt 1562 at 2012-06-05 20:06:00 UTC ***

I had an opportunity to inspect with DOM Inspector an instance of Instantbird suffering from this problem and here is what I saw:
I had a closed group (named "Hidden") that still had 2 contacts (there was "(2)" at the end of the displayed label).
In DOM Inspector, the JS object of the group still had 2 XUL elements in its "contacts" and "contactsById" properties, but these elements were no longer in the DOM tree (their parentNode property was null).

This means that for some reason we called removeChild on some contacts without calling group.removeContact.

After poking at the code for a few minutes, it appears that there's only one place in the code that's likely to ever to that:

group.xml:
234      <method name="updateContactPosition">
[...]
250           // Check if something prevents us from moving the contact now.
251           if (contactElt.hasAttribute("selected") ||
252               contactElt.hasAttribute("open"))
253             this._reorderContactLater(contactElt);
254           else {
255             if (window.getComputedStyle(contactElt).height == "0px")
256               this.parentNode.removeChild(contactElt);
257             else
258               contactElt.state = "collapsing";
259             contactElt.destroy();
260             this.addContact(aSubject);
261           }

I think the problem is that removing the node from the DOM tree detaches the XBL binding, so contactElt.destroy doesn't work.
I'm surprised we never noticed a JS error about contactElt.destroy in the error console, but it's possible it scrolled away within a second (it's likely to happen when connecting/disconnecting accounts, so we have lots and lots of messages scrolling by in the console at that time).

Touching the state property of contact bindings in the group binding is quite ugly. The reason we/I (I don't remember who I should blame for these lines of code) did it is that we want to skip the fading animation when reordering contacts inside a group. I now think that removing the duplicated code by adding a boolean parameter to the contact.removeNode method is much better, and less fragile (that code dealing with animations is tricky, so better limit the places from where we touch it).

The contact property of instances of the contact binding is actually deleted in the destructor, but that code (which used to never be called, before https://bugzilla.mozilla.org/show_bug.cgi?id=83635 got fixed) doesn't call group.removeContact. Again, this is duplicated code that we should clean-up by just calling the destroy method instead (note: that change alone should be enough to fix the bug).
Attachment #8353317 - Flags: review?(clokep)
Assignee: nobody → florian
*** Original post on bio 1377 at 2012-06-05 20:25:19 UTC ***

Blocking 1.2, as it's obviously (now that we know which code is faulty) a regression from bug 954610 (bio 1178).
Whiteboard: [1.2-blocking]
Blocks: 954610
Comment on attachment 8353317 [details] [diff] [review]
Fix

*** Original change on bio 1377 attmnt 1562 at 2012-06-05 21:20:09 UTC ***

Looks good and I tried it out and it seems to work. :)

Not that this greatly concerns me...but just noting that this probably needs to be ported to Thunderbird.
Attachment #8353317 - Flags: review?(clokep) → review+
*** Original post on bio 1377 at 2012-06-05 21:26:10 UTC ***

Setting to assigned.
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86 → All
*** Original post on bio 1377 at 2012-06-05 23:09:29 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/0a1aa7e2827a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.