If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

View All Headers mode leaks DOM nodes

RESOLVED FIXED in Thunderbird 3.0b3

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Alan Wotiz, Assigned: Alan Wotiz)

Tracking

({mlk})

Trunk
Thunderbird 3.0b3

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Created attachment 357318 [details]
Proposed fix
Keywords: mlk

Comment 2

9 years ago
Alan, can you create a real patch for that
https://developer.mozilla.org/en/Getting_your_patch_in_the_tree
Assignee: nobody → ozwye
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 3

9 years ago
Created attachment 357569 [details] [diff] [review]
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.
Attachment #357318 - Attachment is obsolete: true
Comment on attachment 357569 [details] [diff] [review]
Proposed fix

I'll try and take a look at this soon.
Attachment #357569 - Flags: review?(bugzilla)
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.
Attachment #357569 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 6

9 years ago
Created attachment 360246 [details] [diff] [review]
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.
Attachment #357569 - Attachment is obsolete: true
Attachment #360246 - Flags: review?(bugzilla)
(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.
(Assignee)

Comment 8

9 years ago
(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.
(Assignee)

Updated

9 years ago
Attachment #360246 - Attachment is obsolete: true
Attachment #360246 - Flags: review?(bugzilla)
(Assignee)

Comment 9

9 years ago
Created attachment 361017 [details] [diff] [review]
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
// scott@scott-macgregor.org. 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.
Attachment #361017 - Flags: review?(bugzilla)
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.
Attachment #361017 - Flags: review?(bugzilla) → review+
(In reply to comment #9)
> // Warning: if you go to modify any of these JS routines please get a code
> review from
> // scott@scott-macgregor.org. 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.
(Assignee)

Comment 12

9 years ago
Created attachment 364250 [details] [diff] [review]
Updated patch
[Checkin: Comment 13]

Nits fixed, ready for checkin.
Attachment #361017 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/132e294403b0

Thanks for finding and fixing this Alan.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3
Blocks: 481337
Version: unspecified → Trunk
Created attachment 366030 [details] [diff] [review]
(Bv1) Sync after SeaMonkey bug 481337
[Checkin: See comment 17]

Back-port bug 481337 fixes.
Attachment #366030 - Flags: review?(bugzilla)
(Assignee)

Comment 15

9 years ago
Hmmm... I don't see any functional differences from this backport. Maybe not needed here?
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.
Attachment #366030 - Flags: review?(bugzilla) → review+
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).
Attachment #366030 - Attachment description: (Bv1) Sync after SeaMonkey bug 481337 → (Bv1) Sync after SeaMonkey bug 481337 [Checkin: See comment 17]
Attachment #364250 - Attachment description: Updated patch → Updated patch [Checkin: Comment 13]
You need to log in before you can comment on or make changes to this bug.