837845 - Multi-GB memory spike when using regular expressions

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steffen Weihrauch]

Attachment: firefox-memleak.htm testcase

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130130080006

Steps to reproduce:

Using regular expressions with Firefox 19.0 (beta).


Actual results:

Mega Memory Leak. Have tested on two different computers with windows 7, same result. On Firefox 18.0.1 there are all ok (see attachment for testcase).


Expected results:

Same behaviour as firefox 18.0.1 and all other browsers => no memeory leak

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

How are you measuring that it's a memory leak, and how big is the leak in exact terms?

Comment 2

[u279076]

> Firefox 18.0.1
RAM marginally increases, alerts "done" almost immediately

> Firefox 20.0a2
RAM balloons to 3.4GB almost immediately, does not alert "done"
Navigating away from the page drops memory back to normal levels

> Firefox 19.0b4
Ballooning memory while testcase open and browser hangs (needs to be force closed)

> Firefox 19.0a1 2012-11-19 (last mozilla-central-19)
Ballooning memory while testcase open.
Memory is NOT released when navigating away.

The way I see it we have a couple of different issues in Firefox 19 here. 
1. Memory ballooning with the testcase (introduced in Firefox 19)
2. Memory leak after closing testcase ("fixed" in Firefox 20)

I think the priority should be trying to fix the ballooning memory since I think that's the catalyst for this bug. I'll try to find a regression range for this.

Comment 3

[u279076]

Regression range for the memory ballooning to 3.4GB:
* Last Good: 2012-10-11 (99898ec9976a)
* First Bad: 2012-10-12 (90857937b601)
* Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=99898ec9976a&tochange=90857937b601

Unfortunately there is three PGO merges totalling 234 changes in this pushlog and I can't seem to find hourlies going back to this date range.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  In that range, bug 798624 seems like it might be related.

Anthony, would you be willing to complain to the IT folks about the lack of hourlies older than 30 days?  We've been trying to get them to fix that for months with no luck.  :(  Maybe if they heard from someone not on the platform team...

Comment 5

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/99898ec9976a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121010133903
Bad:
http://hg.mozilla.org/mozilla-central/rev/2fae8bd461da
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121011064702
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=99898ec9976a&tochange=2fae8bd461da


Regression window(cached m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/88bb0e2f0373
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121009171503
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0761bc637081
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121009174802
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=88bb0e2f0373&tochange=0761bc637081

Triggered by:
2c08d52e521d	Terrence Cole — Bug 798624 - Specialize low-level character access to JSStableString; r=luke, rs=Waldo Implements JSStableString::chars() and pushes StableCharPtr into the interface at several key choke-points. This will solidly enforce the requirement that we have uninlined jschar*s in places that we may GC with a jschar* on the stack.

Comment 6

[u279076]

Thanks a lot for your help, Alice!

Comment 7

[Terrence Cole [:terrence]]

In today's aurora, after clicking the button the console shows that an out of memory exception was raised.

Timestamp: 2/5/2013 10:21:09 AM
Error: Uncaught asynchronous error: [Exception... "Out of Memory"  nsresult: "0x8007000e (NS_ERROR_OUT_OF_MEMORY)"  location: "JS frame :: resource://gre/modules/sessionstore/_SessionFile.jsm :: task :: line 207"  data: no] at
undefined
Source File: resource://gre/modules/sessionstore/_SessionFile.jsm
Line: 108

Timestamp: 2/5/2013 10:24:31 AM
Error: out of memory
Source File: resource:///modules/sessionstore/SessionStore.jsm
Line: 4169

Refreshing the page afterwards reliably crashes the browser. This should be fun.

Comment 8

[Terrence Cole [:terrence]]

Attachment: v0

Manually backout the API and RegExp aspects of the regressing bug.

Try run is at:
https://tbpl.mozilla.org/?tree=Try&rev=ca79ec726062

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 710328 [details] [diff] [review]
v0

Review of attachment 710328 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsstr.cpp
@@ +2465,1 @@
>              return false;

Let's get rid of this if(!) test, because it's going to look crazy if it stands.

Comment 10

[u279076]

(In reply to Terrence Cole [:terrence] from comment #8)
> Created attachment 710328 [details] [diff] [review]
> v0
> 
> Manually backout the API and RegExp aspects of the regressing bug.
> 
> Try run is at:
> https://tbpl.mozilla.org/?tree=Try&rev=ca79ec726062

I've confirmed with the win32-opt build that the 3.4GB memory spike no longer occurs and the testcase alerts "done" almost immediately. This should be safe to land on the branches.

Comment 11

[Terrence Cole [:terrence]]

Attachment: v1: review comment addressed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 798624
User impact if declined: Memory spikes under string heavy workloads that cross the JSAPI.
Testing completed (on m-c, etc.): Local shell tests pass.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

The Try run for this patch is fairly orange, but this appears to be because I based the patch off of mozilla-beta and we've changed the build infrastructure since then. I'm running another try run of a bare mozilla-beta tree to test  (https://tbpl.mozilla.org/?tree=Try&rev=0c64734b0437), although #releng tells me that orange and worse should be expected here. I'm currently whipping up a similar patch against aurora to Try, which will hopefully work better.

The good news is that this patch is very unlikely to have any adverse impact: if a shell or browser builds and runs at all then it will be exercising the changed code. Since that works fine locally, I think it is most likely fine.

Comment 12

[Terrence Cole [:terrence]]

Try push for Aurora patch, with branding set:
https://tbpl.mozilla.org/?tree=Try&rev=a838077c9603

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 710389 [details] [diff] [review]
v1: review comment addressed

Let's land on Beta and quickly back out if we see new orange, rather than waiting on further builds/tests. Waiting to find out whether we regress anything first may push a fix into beta 6, which is super close to the release.

Comment 14

[Terrence Cole [:terrence]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f22252f3041f

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(I guess disabled is the right flag for when the change is backed out)

Comment 16

[Terrence Cole [:terrence]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f0edb3dc1896

Comment 17

[Terrence Cole [:terrence]]

I think this stuck.

Comment 18

[Terrence Cole [:terrence]]

Attachment: v0: Ported patch forward to aurora, carrying r+.

[Approval Request Comment]
See request for Beta.

Testing completed (on m-c, etc.): One day on mozilla-beta.

Comment 19

[Virgil Dicu [:virgil] [QA]]

Verified the memory leak on Firefox 19.0 beta 4 following the steps from comment 0.

There is no memory leak or hang for Firefox 19.0 beta 5. 
After loading the testcase with large amount of data and click "test" the pop-up with "done" appears immediately.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130206083616

Comment 20

[Steffen Weihrauch]

Just great, thanks to all. Seems to be stable at the moment with version 19.0.0.4875 (sorry i don't know how to get the exact version).

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 710980 [details] [diff] [review]
v0: Ported patch forward to aurora, carrying r+.

I know we're doing it a little backwards on this bug what with landing to Beta first, but now that we know it's working as intended let's get this to Aurora and Trunk pronto.

Comment 22

[Terrence Cole [:terrence]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1939e049e60d

Comment 23

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b6cf8fe73e0

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b6cf8fe73e0

Comment 25

[u279076]

Dave, this seems like it might be a good candidate for a Mozmill Endurance test.

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #25)
> Dave, this seems like it might be a good candidate for a Mozmill Endurance
> test.

The endurance tests are for typically for repeating a snippet of user interaction (open a window/tab/bookmark) over and over whilst gathering metrics. That said, I'd be happy to help out if someone wants to work on an endurance test for this.

Comment 27

[Virgil Dicu [:virgil] [QA]]

No memory leak or hang for Firefox 20.0 beta 1. 
After loading the testcase with large amount of data and click "test" the pop-up with "done" appears immediately and memory usage remains at the same level.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 (20130220104816)

Comment 28

[u279076]

(In reply to Dave Hunt (:davehunt) from comment #26)
> I'd be happy to help out if someone wants to work on an
> endurance test for this.

Thanks Dave. I've gone ahead and filed bug 848577. I'm not sure if you have any whiteboard tags for mentorship or the like.

Comment 29

[Jason Smith [:jsmith]]

Question - Why is a Mozmill test appropriate here? Why not use a TBPL-based automation framework here?

Comment 30

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Jason Smith [:jsmith] from comment #29)
> Question - Why is a Mozmill test appropriate here? Why not use a TBPL-based
> automation framework here?

You're right, we should only use Mozmill if there is no suitable alternative. Did you have one in mind Jason? As far as I know talos and AWSY only measures metrics for page loads.

Comment 31

[Jason Smith [:jsmith]]

(In reply to Dave Hunt (:davehunt) from comment #30)
> (In reply to Jason Smith [:jsmith] from comment #29)
> > Question - Why is a Mozmill test appropriate here? Why not use a TBPL-based
> > automation framework here?
> 
> You're right, we should only use Mozmill if there is no suitable
> alternative. Did you have one in mind Jason? As far as I know talos and AWSY
> only measures metrics for page loads.

Based on what I'm reading on this bug, this is a memory spike due to a memory leak in the JS engine with regular expressions, right?

If it's just a leak with the existing test case attached as the point of verification, I know a crashtest would be a potential option to use here, as that runs in TBPL and does memory leak checking automatically. That would be really simple to implement as well in alignment with the attached test case given here.

Comment 32

[Jason Smith [:jsmith]]

Moving discussion to bug 845577 on test framework choice.

Comment 33

[Terrence Cole [:terrence]]

(In reply to Jason Smith [:jsmith] from comment #32)
> Moving discussion to bug 845577 on test framework choice.

I'm not sure that's the right bug you linked.

In any case, this bug wasn't a leak, it was an expected increase in memory usage; it just happened to be much more drastic than desired in this particular case. Given that the regressing code is gone now (and it was a very specific piece of code), I don't think a test is really warranted here.

Comment 34

[Jason Smith [:jsmith]]

(In reply to Terrence Cole [:terrence] from comment #33)
> (In reply to Jason Smith [:jsmith] from comment #32)
> > Moving discussion to bug 845577 on test framework choice.
> 
> I'm not sure that's the right bug you linked.

Ack. Meant to say bug 848577.

> 
> In any case, this bug wasn't a leak, it was an expected increase in memory
> usage; it just happened to be much more drastic than desired in this
> particular case. Given that the regressing code is gone now (and it was a
> very specific piece of code), I don't think a test is really warranted here.

Fair enough. Minusing then on that basis.

Comment 35

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0

No memory leak or hang for Firefox 21.0 beta 3 (buildID: 20130416200523).
After loading the source of the forum in the box and test is clicked, I see a 10 MB memory increase in Task Manager, I think that is reasonable since there is so much code loading.