Note: There are a few cases of duplicates in user autocompletion which are being worked on.

JavaScript toLocaleString has a nasty memory leak

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: goberman, Assigned: bz)

Tracking

(4 keywords)

unspecified
mozilla2.0b8
dev-doc-complete, mlk, verified1.9.1, verified1.9.2
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 -, status2.0 wanted, status1.9.2 .14-fixed, status1.9.1 .17-fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Component: Developer Tools → JavaScript Engine
Product: Firefox → Core
Created attachment 495703 [details]
Testcase from the reporter
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.
Severity: major → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: mlk
Assignee: nobody → general
QA Contact: developer.tools → general
(Assignee)

Comment 3

7 years ago
At least on trunk, you're hitting bug 617215 in addition to whatever is going in in 1.9.2.
Depends on: 617215
(Assignee)

Comment 4

7 years ago
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?
blocking2.0: --- → ?
OS: Windows XP → All
Hardware: x86 → All
(Assignee)

Comment 5

7 years ago
Brendan, see comment 4.
(Assignee)

Comment 6

7 years ago
Created attachment 495732 [details] [diff] [review]
part 1.  Fix Number.toLocaleString leak.
Attachment #495732 - Flags: review?(brendan)
(Assignee)

Comment 7

7 years ago
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!
(Assignee)

Comment 8

7 years ago
Created attachment 495737 [details] [diff] [review]
part 2.  Make the localeToUnicode callback take |const char*|.
Attachment #495737 - Flags: review?(brendan)
(Assignee)

Comment 9

7 years ago
Created attachment 495739 [details] [diff] [review]
1.9.1/1.9.2 merge of part 1 only
Attachment #495739 - Flags: review?(brendan)
(Assignee)

Updated

7 years ago
Assignee: general → bzbarsky
Whiteboard: [need review]
Comment on attachment 495732 [details] [diff] [review]
part 1.  Fix Number.toLocaleString leak.

dev-doc-needed?

/be
Attachment #495732 - Flags: review?(brendan) → review+
Attachment #495737 - Flags: review?(brendan) → review+
Attachment #495739 - Flags: review?(brendan) → review+
(Assignee)

Comment 11

7 years ago
> dev-doc-needed?

Yes, we need to document that the localeToUnicode callback shouldn't free the buffer it's given.
Keywords: dev-doc-needed
(Assignee)

Updated

7 years ago
Whiteboard: [need review] → [need approval]
(Assignee)

Comment 12

7 years ago
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.
Attachment #495732 - Flags: approval2.0?
(Assignee)

Updated

7 years ago
Attachment #495739 - Flags: approval1.9.2.14?
Attachment #495739 - Flags: approval1.9.1.17?

Comment 13

7 years ago
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
Attachment #495739 - Flags: approval1.9.2.14?
Attachment #495739 - Flags: approval1.9.2.14+
Attachment #495739 - Flags: approval1.9.1.17?
Attachment #495739 - Flags: approval1.9.1.17+

Updated

7 years ago
Attachment #495732 - Flags: approval2.0? → approval2.0+

Updated

7 years ago
blocking2.0: ? → -
status2.0: --- → wanted
(Assignee)

Updated

7 years ago
Whiteboard: [need approval] → [need landing]
(Assignee)

Comment 14

7 years ago
Pushed:

  http://hg.mozilla.org/mozilla-central/rev/abef6c3c5dbf
  http://hg.mozilla.org/mozilla-central/rev/f971ad6ed5a5

  http://hg.mozilla.org/releases/mozilla-1.9.2/rev/10d9c0bb3a89

  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a7fc1bc50090

and a 1.9.1 bustage fix as:

  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1e2a21dcd6de
Status: NEW → RESOLVED
Last Resolved: 7 years ago
status1.9.1: --- → .17-fixed
status1.9.2: --- → .14-fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b8
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.
Keywords: dev-doc-needed → dev-doc-complete
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.
Keywords: verified1.9.1, verified1.9.2
You need to log in before you can comment on or make changes to this bug.