User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3b) Gecko/20030104 Build Identifier: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3b) Gecko/20030104 When a script is compiled with (some kinds of) bad regular expressions, memory is leaked. My test leaks 40 bytes when the input /x*{/ is fed into the regular expression compiler. When an error happens in ParseQuantAtom in jsregexp.c, it just returns NULL immediately, without cleaning up allocated RE nodes held only in local variables. Reproducible: Always Steps to Reproduce: 1. Run the JS Shell under Purify, in order to detect memory leaks. 2. Type "mystring".replace(/x*{/, ''); 3. After syntax error is reported, scan for memory leaks. Actual Results: 40 bytes leaked (in 2 regexp nodes) Expected Results: No leakage.

This is a Purify log and JSShell transcript demonstrating/documenting the leak. (I just read the "keyword" help page that said I should attach this if I claimed a memory leak.) --scole

If I recall correctly, rogerl is working on a wholesale replacement for jsregexp.c. Is that still the case? I ask, because I wonder if it's worth my while to try and repair this bug, or if I should wait until that other project is done. --scole

I don't think rogerl's changes fix this. The old, bytecoded-NFA codebase used an arena to allocate RENodes, which were temporary data structures used only during compilation from regexp source to NFA bytecode. It still seems like we win by using cx->tempPool instead of raw malloc for RENodes. Roger, what do you think? /be

Yes, the big new mega patch will fix this problem; it does all the RENode allocations out of cx->tempPool and are blown away (error or not) after the bytecode is generated. I'm also using a pool for data allocations during the RE execution and it's a big win there, too.

Good news! I guess I just need to sit patiently and bite my nails :) Memory leaks are always critical on our "must fix" lists, since they lead to performance degradation over time (especially on embedded systems); it's nice to know that an architectural change will take all these away without having to stumble across them one by one. Roger, do you have an ETA on this? (I might try and fix this stuff locally, depending on schedules, but I hope I won't have to.) --scole

Duh. I don't need to fix this myself, I just need to take the patch in bug 85721. Should we dup this bug against that one? They're not really the same, but this bug will be fixed by that one. (Duplicate, rather than dependency, because no work needs to be done here, and then we can go and forget about this one.) --scole

Don't dup, make this bug depend on bug 85721. Duplicate means the two bugs describe the same problem, not the same (or an overlapping) solution. /be

As the solution presented for bug 85721 will solve this problem, this bug is now depends on that one. --scole

I've confirmed that the patch in bug 85721 removes this memory leak, but it's due to the fact that my "bad regexp expression" is now valid. (A different bad regex, /abc*[/, does not leak, however, so I'm willing to believe that if /x*{/ were invalid, it wouldn't leak either.) --scole

Attachment 111524 [details] from bug 85721 now perfectly fixes this bug. (I.e.: /x*{/ is an error again, and it doesn't cause memory leaks either.) Thanks again, Roger. --scole

Testcase added to JS testsuite: mozilla/js/tests/ecma_3/RegExp/regress-188206.js As suggested by Roger in bug 85721 comment 53, this deliberately misuses regexp quantifiers and checks for SyntaxErrors to result -

Note that the original test case /x*{/ is now being made legal (again), see bug 1906850 for what happenned to cause this.

Oops - that's bug 190685

Fixed by checkin of the patch in bug 85721. --scole

Rubber-stamp vrfy -

ecma_3/RegExp/regress-188206.js fails sections 13, 14, 17, 20-27 in today's sm and ff trunk and 1.7x

oops, wrong bug.

(In reply to comment #16) > ecma_3/RegExp/regress-188206.js > fails sections 13, 14, 17, 20-27 in today's sm and ff trunk and 1.7x fixed by bug 289628