391587 - js_FinishDeflatedStringCache leaks the strings for interned atoms on shutdown

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 3

[Brendan Eich [: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

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 276060 [details] [diff] [review]
patch

Should I add a blank line after as well?

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 276060 [details] [diff] [review]
patch

Sure, after too is ok, not strictly necessary either.

/be

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2007-08-10 14:30.