js_FinishDeflatedStringCache leaks the strings for interned atoms on shutdown

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({memory-leak})

Trunk
mozilla1.9alpha8
memory-leak
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
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 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 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.