Closed
Bug 397231
Opened 17 years ago
Closed 17 years ago
nsMicrosummaryService leaks when loading malformed page as XML (in particular when loading file:/// HTML pages)
Categories
(Firefox Graveyard :: Microsummaries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 3 beta1
People
(Reporter: asqueella, Assigned: myk)
Details
(Keywords: memory-leak)
Attachments
(2 files)
|
29.73 KB,
text/plain
|
Details | |
|
2.35 KB,
patch
|
mconnor
:
review+
mconnor
:
approval1.9+
|
Details | Diff | Splinter Review |
Steps to reproduce:
0. Create c:\1.html with malformed XML in it (e.g. <html><body>aaa)
1. Open Firefox, open Error Console
2. Run the following:
var uri = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService).newURI("file:///c:/1.html", null, null);var ms=Components.classes["@mozilla.org/microsummary/service;1"].getService(Components.interfaces.nsIMicrosummaryService).getMicrosummaries(uri,-1);
Actual results: lots of stuff leaked, exception about "contains invalid XML" in the Error Console
Expected: no leaks.
If I add a |this.destroy()| before the |throw| here, the leaks go away:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js&rev=1.75#2068
The main reason this is a problem is that microsummary code gets called when an item is selected in the places organizer. So whenever I select a file:/// place in organizer, we leak.
|
Reporter | |
Comment 1•17 years ago
|
||
Requesting blocking for the places organizer leak mentioned in the last paragraph of comment 0.
Flags: blocking-firefox3?
|
Assignee | |
Updated•17 years ago
|
Assignee: nobody → myk
Priority: -- → P1
Target Milestone: --- → Firefox 3 M9
|
|
Assignee | |
Comment 2•17 years ago
|
||
We shouldn't be throwing once the network load completes, we should just call the error callback if there's an error parsing the result (or if we can't parse it). Here's a patch that replaces the two throws in the load handler (and its entrained functions) with calls to the error handler.
This should fix the problem, but I'm still waiting for my debug build to rebuild before I can confirm it, so holding off on requesting review until then.
|
|
Assignee | |
Comment 3•17 years ago
|
||
Comment on attachment 282767 [details] [diff] [review]
patch v1: replaces throws with calls to error handler
Yup, this patch fixes the leak without compromising functionality in my tests. Requesting review.
Attachment #282767 -
Flags: review?(mconnor)
|
||
Comment 4•17 years ago
|
||
Comment on attachment 282767 [details] [diff] [review]
patch v1: replaces throws with calls to error handler
looks good to me, r=mconnor
Attachment #282767 -
Flags: review?(mconnor) → review+
|
|
Assignee | |
Comment 5•17 years ago
|
||
Comment on attachment 282767 [details] [diff] [review]
patch v1: replaces throws with calls to error handler
Requesting approval for this leak fix. The risk of this patch is contained because it only changes functionality in unusual cases (i.e. when a load doesn't succeed because of malformed XML or because there is no browser window open), so I think this is low risk.
Attachment #282767 -
Flags: approval1.9?
|
|
||
Updated•17 years ago
|
Attachment #282767 -
Flags: approval1.9? → approval1.9+
|
|
Assignee | |
Comment 6•17 years ago
|
||
Checking in browser/components/microsummaries/src/nsMicrosummaryService.js;
/cvsroot/mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js,v <-- nsMicrosummaryService.js
new revision: 1.79; previous revision: 1.78
done
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
|
||
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
|
||
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•