Closed Bug 617097 Opened 14 years ago Closed 14 years ago

JavaScript toLocaleString has a nasty memory leak

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b8
Tracking Flags:
Tracking Status
blocking2.0 --- -
status2.0 --- wanted
status1.9.2 --- .14-fixed
status1.9.1 --- .17-fixed

People

(Reporter: goberman, Assigned: bzbarsky)

References

Details

(4 keywords)

Attachments

(4 files)

Testcase from the reporter
569 bytes, text/html
Details
part 1. Fix Number.toLocaleString leak.
1.08 KB, patch
brendan
: review+
sayrer
: approval2.0+
Details | Diff | Splinter Review
part 2. Make the localeToUnicode callback take |const char*|.
1.56 KB, patch
brendan
: review+
Details | Diff | Splinter Review
1.9.1/1.9.2 merge of part 1 only
764 bytes, patch
brendan
: review+
christian
: approval1.9.2.14+
christian
: approval1.9.1.17+
Details | Diff | Splinter Review 
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.
Component: Developer Tools → JavaScript Engine
Product: Firefox → Core
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
At least on trunk, you're hitting bug 617215 in addition to whatever is going in in 1.9.2.
Depends on: 617215
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
Brendan, see comment 4.

        Attachment #495732 -
        Flags: review?(brendan)

    
    

    
  
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!

    
    

    
  


  

        Attachment #495739 -
        Flags: review?(brendan)

    
  
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+

    
    

    
  
> dev-doc-needed?

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

    
  
Whiteboard: [need review] → [need approval]

    
    

    
  
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?

    
  

        Attachment #495739 -
        Flags: approval1.9.2.14?

        Attachment #495739 -
        Flags: approval1.9.1.17?

    
    

    
  
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+

    
  

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

    
  
blocking2.0: ? → -

          status2.0:
          --- → wanted

    
  
Whiteboard: [need approval] → [need landing]

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: