js_FinishDeflatedStringCache leaks the strings for interned atoms on shutdown
RESOLVED
FIXED
in mozilla1.9alpha8
Status
()
People
(Reporter: dbaron, Assigned: dbaron)
Tracking
({memory-leak})
Firefox Tracking Flags
(Not tracked)
Details
Attachments
(1 attachment)
|
1.17 KB,
patch
|
brendan
:
review+
brendan
:
approval1.9+
|
Details | Diff | Splinter Review |
js_FinishDeflatedStringCache doesn't free the strings owned by rt->deflatedStringCache; it only frees the table. This shows up as a rather large number of shutdown leaks (pretty easy to spot now that trace-malloc runs on Windows). It seems like the best fix would be to use a non-default set of JSHashAllocOps (although it's a bit of a pain for overriding just one of them) and then make js_PurgeDeflatedStringCache not call free manually (and probably just use Remove rather than RawRemove).
|
(Assignee) | |
Comment 1•11 years ago
|
||
So, brendan pointed out that what I said above shouldn't happen because the strings should have all been freed. The problem is the order in which we call js_FinishDeflatedStringCache and js_FinishAtomState; js_FinishAtomState finalizes strings, so we shouldn't finish the deflated string cache until after it has run. Patch coming shortly, after testing that it fixes the leak.
Assignee: general → dbaron
Summary: js_FinishDeflatedStringCache leaks the strings in the cache on shutdown → js_FinishDeflatedStringCache leaks the strings for interned atoms on shutdown
Target Milestone: --- → mozilla1.9 M8
|
|
(Assignee) | |
Comment 2•11 years ago
|
||
Created attachment 276060 [details] [diff] [review] patch
Attachment #276060 -
Flags: review?(brendan)
|
||
Comment 3•11 years ago
|
||
Comment on attachment 276060 [details] [diff] [review] patch Nit: one blank line before a comment that takes up one or more whole lines. Thanks for patching! /be
Attachment #276060 -
Flags: review?(brendan) → review+
|
|
(Assignee) | |
Comment 4•11 years ago
|
||
Comment on attachment 276060 [details] [diff] [review] patch Should I add a blank line after as well?
Attachment #276060 -
Flags: approval1.9?
|
|
||
Comment 5•11 years ago
|
||
Comment on attachment 276060 [details] [diff] [review] patch Sure, after too is ok, not strictly necessary either. /be
Attachment #276060 -
Flags: approval1.9? → approval1.9+
|
|
(Assignee) | |
Comment 6•11 years ago
|
||
Fix checked in to trunk, 2007-08-10 14:30.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
|
||
Updated•11 years ago
|
||
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•