Last Comment Bug 617097 - JavaScript toLocaleString has a nasty memory leak
: JavaScript toLocaleString has a nasty memory leak
Status: RESOLVED FIXED
: dev-doc-complete, mlk, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla2.0b8
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 617215
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-06 12:26 PST by goberman
Modified: 2011-01-04 16:39 PST (History)
7 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wanted
.14-fixed
.17-fixed


Attachments
Testcase from the reporter (569 bytes, text/html)
2010-12-06 17:14 PST, Matthias Versen [:Matti]
no flags Details
part 1. Fix Number.toLocaleString leak. (1.08 KB, patch)
2010-12-06 18:19 PST, Boris Zbarsky [:bz]
brendan: review+
sayrer: approval2.0+
Details | Diff | Review
part 2. Make the localeToUnicode callback take |const char*|. (1.56 KB, patch)
2010-12-06 18:28 PST, Boris Zbarsky [:bz]
brendan: review+
Details | Diff | Review
1.9.1/1.9.2 merge of part 1 only (764 bytes, patch)
2010-12-06 18:29 PST, Boris Zbarsky [:bz]
brendan: review+
christian: approval1.9.2.14+
christian: approval1.9.1.17+
Details | Diff | Review

Description goberman 2010-12-06 12:26:31 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 (.NET CLR 3.5.30729)

I have an application that calls toLocaleString frequently to format prices. I have been chasing a memory leak in it for a while and isolated it to toLocaleString. This function does not leak memory in other browsers.

Very easy to duplicate: just open a page below and watch memory. It will allocate hundreds of megabytes within minutes.

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/1999/REC-html401-19991224/loose.dtd">
<html>
<head>
    <title>test</title>
</head>
<body onload="initialize();">
    <input type="button" value="TEST" onclick="test(); return false;" />

    <script type="text/javascript">

        function test() {
            while (true) {
                format(100000.12345);
            }
        };

        function format(data) {
            return data.toLocaleString();
        };

    </script>

</body>
</html>

Reproducible: Always

Steps to Reproduce:
1.Open page in FF:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/1999/REC-html401-19991224/loose.dtd">
<html>
<head>
    <title>test</title>
</head>
<body onload="initialize();">
    <input type="button" value="TEST" onclick="test(); return false;" />

    <script type="text/javascript">

        function test() {
            alert('test')
            while (true) {
                format(100000.12345);
            }
        };

        function format(data) {
            return data.toLocaleString();
        };

    </script>

</body>
</html>

2. Watch memory in Task Manager. Memory leak is easily observed.

Actual Results:  
Huge memory leak.

Expected Results:  
Did not expect any memory leaks.
Comment 1 Matthias Versen [:Matti] 2010-12-06 17:14:01 PST
Created attachment 495703 [details]
Testcase from the reporter
Comment 2 Matthias Versen [:Matti] 2010-12-06 17:19:03 PST
I can confirm a memory issue with Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101206 Firefox/4.0b8pre SeaMonkey/2.1b2pre

Using the testcase and continuing a few times at the slow script warning and my fresh started SM is using 1GB ram and it doesn't seem to give the memory back.
Comment 3 Boris Zbarsky [:bz] 2010-12-06 17:37:53 PST
At least on trunk, you're hitting bug 617215 in addition to whatever is going in in 1.9.2.
Comment 4 Boris Zbarsky [:bz] 2010-12-06 17:56:33 PST
num_toLocaleString assumes that the context's localeToUnicode callback takes ownership (or at least frees?) the passed-in buffer.  The DOM's LocaleToUnicode does no such thing.  So we leak the buffer.

Brendan, which side do we need to fix this on?  Easy enough to fix on the DOM side, in which case the failure mode for embedders who guess wrong is a leak instead of the double-free if I fix this on the JS side... but maybe we have docs somewhere that would prevent people from guessing wrong?
Comment 5 Boris Zbarsky [:bz] 2010-12-06 17:56:46 PST
Brendan, see comment 4.
Comment 6 Boris Zbarsky [:bz] 2010-12-06 18:19:40 PST
Created attachment 495732 [details] [diff] [review]
part 1.  Fix Number.toLocaleString leak.
Comment 7 Boris Zbarsky [:bz] 2010-12-06 18:20:11 PST
That patch fixes on the JS side, since jsdate passes a _stack_ buffer to the toLocaleString callback, so I'd bet no one is calling cx->free on it already!
Comment 8 Boris Zbarsky [:bz] 2010-12-06 18:28:34 PST
Created attachment 495737 [details] [diff] [review]
part 2.  Make the localeToUnicode callback take |const char*|.
Comment 9 Boris Zbarsky [:bz] 2010-12-06 18:29:41 PST
Created attachment 495739 [details] [diff] [review]
1.9.1/1.9.2 merge of part 1 only
Comment 10 Brendan Eich [:brendan] 2010-12-08 12:32:36 PST
Comment on attachment 495732 [details] [diff] [review]
part 1.  Fix Number.toLocaleString leak.

dev-doc-needed?

/be
Comment 11 Boris Zbarsky [:bz] 2010-12-08 12:46:11 PST
> dev-doc-needed?

Yes, we need to document that the localeToUnicode callback shouldn't free the buffer it's given.
Comment 12 Boris Zbarsky [:bz] 2010-12-08 12:46:44 PST
Comment on attachment 495732 [details] [diff] [review]
part 1.  Fix Number.toLocaleString leak.

Looking for approval for this leak fix.  Risk is low, I think.
Comment 13 christian 2010-12-08 16:20:02 PST
Comment on attachment 495739 [details] [diff] [review]
1.9.1/1.9.2 merge of part 1 only

a=LegNeato for 1.9.2.14 and 1.9.1.17
Comment 15 Eric Shepherd [:sheppy] 2010-12-10 09:31:37 PST
The locale callbacks aren't documented yet, but a note has been added to the docs not to free this memory. I don't know if devs will find it, but it's out there.
Comment 16 Al Billings [:abillings] 2011-01-04 16:39:59 PST
Verified for 1.9.1 in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.17pre) Gecko/20110104 Shiretoko/3.5.17pre ( .NET CLR 3.5.30729) and for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.14pre) Gecko/20110104 Namoroka/3.6.14pre ( .NET CLR 3.5.30729). The massive memory ballooning that was happening pre-fix is not occurring post-fix.

Note You need to log in before you can comment on or make changes to this bug.