Closed Bug 48030 Opened 24 years ago Closed 24 years ago

leak parser leaving multi-mixed page by going to other page

Categories

(Core :: DOM: HTML Parser, defect, P1)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: dbaron, Assigned: harishd)

Details

(Keywords: memory-footprint, memory-leak, Whiteboard: [nsbeta3+][pdtp1])

Attachments

(3 files)

a --comptr balance tree when exiting after loading the page (no leak)
23.05 KB, text/plain
Details
a --comptr balance tree when moving from multi-mixed page to about:blank (leak)
18.71 KB, text/plain
Details
patch to avoid circular ownership in the first place
11.10 KB, patch
Details | Diff | Splinter Review
DESCRIPTION: We leak a parser and content sink and everything they own (i.e., the document) when we load a multi-mixed page and leave that page by going to another page rather than exiting the browser. We leak because the parser leaks when DidBuildModel is not called. STEPS TO REPRODUCE: 1. load http://bugzilla.mozilla.org/ 2. click on "View Bugs Already Reported Today" 3. load about:blank I will attach leak logs showing the case where we leak (the above steps), and the same parser where I replace (3) above with immediately exiting the browser. Exiting the browser causes DidBuildModel to be called by the following chain: nsDocShell::Destroy nsDocShell::Stop DocumentViewerImpl::Stop nsHTMLDocument::StopDocumentLoad nsParser::Terminate nsParser::DidBuildModel This doesn't happen when moving to another page. It's not clear to me where the problem is, but this is a big leak. I wonder if some of the Stop methods should normally be called. I also wonder if there's something strange about the multi-mixed converter.
Nominating for nsbeta3 since this is a big leak and assigning to harish since rickg is away.
Assignee: rickg → harishd
Keywords: footprint, mlk, nsbeta3
someone else pointed out a case in which DidBuildModel was not being called. This is big.
there we go: 25368 . Sounds like DidBuildModel() call semantics need to change.
Whiteboard: [nsbeta3+]
Harish, what do you think of these changes? I do need to test them a good bit more. They make it so that rather than silently leak if DidBuildModel is called, we assert instead (and don't leak). The previous ownership model was (as I understand it): document owns parser parser owns content sink content sink owns parser content sink owns document I made the latter two weak references.
would this patch solve 25368 as well?
Probably. How does one hand the parser a non-existant URL?
Your patch looks good to me. Weak reference would definitely break the circularity but rememeber that it's a big HACK by itself. Post release we should find a better way to breakup the circularity ( IMO we should avoid using weak references as much as possible ). BTW, Good work David in fixing this leak and the 1M (!!) leak.
Harish, give a yell if "Your patch looks good to me." is not "r=harishd"... See also bug 37434.
"Your patch looks good to me." is "r=harishd" :-) Go ahead.
Actually, I won't check it in because I discovered my patch causes http://www.netscape.com/ to crash. I'll add more details to bug 37434 (which I think is the appropriate bug).
Setting priority to P1.
Status: NEW → ASSIGNED
Priority: P3 → P1
PDT agrees P1
Putting [pdtp1] in whiteboard
Whiteboard: [nsbeta3+] → [nsbeta3+][pdtp1]
Rick Potts recent checkin to the docshell fixed this leak. Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
updated qa contact.
QA Contact: janc → bsharma
Verified on: build: 2001-03-29-09-Mtrunk Platform: Win NT Marked verified as per developer comments, since cannot verify the leaks in the parser.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: