Closed Bug 473905 Opened 16 years ago Closed 16 years ago

View All Headers mode leaks DOM nodes

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: aw.moz01, Assigned: aw.moz01)

References

Details

(Keywords: memory-leak)

Attachments

(2 files, 4 obsolete files)

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.
Attached file Proposed fix (obsolete) —
Keywords: mlk
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
Attached patch Proposed fix (obsolete) — Splinter Review
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-
Attached patch Revised proposed fix (obsolete) — Splinter Review
(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.
(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.
Attachment #360246 - Attachment is obsolete: true
Attachment #360246 - Flags: review?(bugzilla)
Attached patch Revised patch (obsolete) — Splinter Review
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.
Nits fixed, ready for checkin.
Attachment #361017 - Attachment is obsolete: true
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/132e294403b0 Thanks for finding and fixing this Alan.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3
Version: unspecified → Trunk
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+
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.

Attachment

General

Creator:
Created:
Updated:
Size: