473905 - View All Headers mode leaks DOM nodes

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Alan Wotiz]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090115 Shredder/3.0b2pre

Selecting View All Headers mode creates new DOM nodes under 'variousHeadersBox' for all headers in the current message. Returning to View Normal Headers mode deletes the pointers to the nodes, but not the nodes themselves. Each time View All Headers mode is entered, a new set of nodes is created with the same names as already existing ones.

Reproducible: Always

Steps to Reproduce:
1. Display an incoming message
2. Select View All Headers mode
3. Examine nodes under 'variousHeadersBox' using DOM inspector
4. Select View Normal Headers, then View All Headers again
5. Number of nodes increases each time step 4 is repeated
Actual Results:  
(see above)

Expected Results:  
Nodes should be deleted when their pointers are deleted.

There are two possible solutions to this - either delete the added nodes when returning to View Normal Headers mode, or re-use any existing nodes rather than recreating them.

Re-using the nodes causes an additional problem of messing up the order that the additional headers are displayed for the first message (see bug 473903 for a better explanation)

A proposed fix that deletes the added nodes follows.

This bug is also in TB2.0.0.19 - the same fix applies.

Comment 1

[Alan Wotiz]

Attachment: Proposed fix

Comment 2

[Magnus Melin [:mkmelin]]

Alan, can you create a real patch for that
https://developer.mozilla.org/en/Getting_your_patch_in_the_tree

Comment 3

[Alan Wotiz]

Attachment: Proposed fix

I'm afraid I don't have any of the dev tools installed, but I believe this patch is in the right format anyway.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 357569 [details] [diff] [review]
Proposed fix

I'll try and take a look at this soon.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 357569 [details] [diff] [review]
Proposed fix

To me, this seems much more like EnsureMinimumNumberOfHeaders() isn't doing its job properly - i.e. reusing unused nodes - than needing to clear out the nodes before we switch.

It would probably be worth poking around the indexes in the headerTable and see what is happening around there.

Comment 6

[Alan Wotiz]

Attachment: Revised proposed fix

(from comment #5)
Actually, no. EnsureMinimumNumberOfHeaders() only deals with the "dummy" headers, not the "real" ones added in All Headers mode. It's not even called if the mailnews.headers.minNumHeaders pref is 0 (the default value). In any case, it's called with headerTable = gExpandedHeaderView, but gExpandedHeaderView has already been cleared by that point (in messageHeaderSink.onStartHeaders), which is what causes the original problem.

However, your comment did lead me to notice that I should be clearing the dummy header index that's used by EnsureMinimumNumberOfHeaders(), though it's little more than an aesthetic change. This is in the revised patch.

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> Created an attachment (id=360246) [details]
> Revised proposed fix
> 
> (from comment #5)
> Actually, no. EnsureMinimumNumberOfHeaders() only deals with the "dummy"
> headers, not the "real" ones added in All Headers mode. It's not even called if
> the mailnews.headers.minNumHeaders pref is 0 (the default value). In any case,
> it's called with headerTable = gExpandedHeaderView, but gExpandedHeaderView has
> already been cleared by that point (in messageHeaderSink.onStartHeaders), which
> is what causes the original problem.

Ok, thanks for the explanation.

I think what I'd prefer is rather than just deleting everything after expandeduser-agentBox would be if you iterated through gExpandedHeaderView and removed the dom item that is stored in the enclosingBox member.

You'd have to be careful not to remove the ones for the predefined headers, but it would be much better than hooking off an existing id, which we may change (to a different field) or an extension try to add something after it.

Comment 8

[Alan Wotiz]

(In reply to comment #7)
> I think what I'd prefer is rather than just deleting everything after
> expandeduser-agentBox would be if you iterated through gExpandedHeaderView and
> removed the dom item that is stored in the enclosingBox member.
>
> You'd have to be careful not to remove the ones for the predefined headers, but
> it would be much better than hooking off an existing id, which we may change
> (to a different field) or an extension try to add something after it.

You're right - that does make more sense. I don't see an existing reliable way to tell a predefined element from a created one; I think the safest way is to add a new flag to just the gExpandedHeaderView elements that are created on the fly (e.g. anything added by createNewHeaderView()). Then only those flagged elements will have their dom items removed later.

I should have some time in the next few days to try this out.

Comment 9

[Alan Wotiz]

Attachment: Revised patch

This keeps track of which headers are newly added, and doesn't touch any of the predefined ones or any inserted from an addon.

I noticed the following comment at the top of msgHdrViewOverlay.js:

// Warning: if you go to modify any of these JS routines please get a code review from
// [e-mail]. It's critical that the code in here for displaying
// the message headers for a selected message remain as fast as possible.

Does that review apply in this case? The only performance issue is a slight added delay when switching from All Headers to Normal Headers mode (same as the existing delay when going from Normal to All Headers), but there shouldn't be any other effect on header display times.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 361017 [details] [diff] [review]
Revised patch

Sorry for the delay in reviewing this.

>           hideHeaderView(gExpandedHeaderView);
>+          removeNewHeaderView(gExpandedHeaderView);

I'd have liked a slightly clearer name, but couldn't think of one and it matches with createNewHeaderView so its quite sensible. However, I think it should be removeNewHeaderViews as its removing more than one.

>+  for (index in headerTable)
>+  {
>+    var headerEntry = headerTable[index];
>+    if ("isNewView" in headerEntry && headerEntry.isNewView)

nit: you only need if (headerEntry.isNewView) here. If its not in headerEntry then it will be "undefined" which equates to false. If it is in there and false, then it will still be false :-)

>+      var headerNode = headerEntry.enclosingBox;
>+      headerNode.parentNode.removeChild(headerNode);

nit: no need for the intermediate variable. If you want to avoid wrapping too far then do:

headerEntry.enclosingBox.parentNode
           .removeChild(headerNode);

r=me with those fixed.

If you can update the patch and attach it to the bug, then we'll check it in for you.

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #9)
> // Warning: if you go to modify any of these JS routines please get a code
> review from
> // [e-mail]. It's critical that the code in here for
> displaying
> // the message headers for a selected message remain as fast as possible.
> 
> Does that review apply in this case?

Scott's not on the project any more, and its an old comment that we should probably remove/revise at some stage.

Comment 12

[Alan Wotiz]

Attachment: Updated patch [Checkin: Comment 13]

Nits fixed, ready for checkin.

Comment 13

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/132e294403b0

Thanks for finding and fixing this Alan.

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Sync after SeaMonkey bug 481337 [Checkin: See comment 17]

Back-port bug 481337 fixes.

Comment 15

[Alan Wotiz]

Hmmm... I don't see any functional differences from this backport. Maybe not needed here?

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 366030 [details] [diff] [review]
(Bv1) Sync after SeaMonkey bug 481337
[Checkin: See comment 17]

>           hideHeaderView(gExpandedHeaderView);
>-          removeNewHeaderViews(gExpandedHeaderView);
>+          RemoveNewHeaderViews(gExpandedHeaderView);

Please don't change the capitilisation of this function. It matches the general trend in this file and what most people prefer for js.

r=me with that fixed.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 366030 [details] [diff] [review]
(Bv1) Sync after SeaMonkey bug 481337
[Checkin: See comment 17]


http://hg.mozilla.org/comm-central/rev/3894f67008e0
with comment 16 suggestion(s).