Closed
Bug 626118
Opened 13 years ago
Closed 13 years ago
String.match leaking
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: extensiondevelopment, Assigned: cdleary)
References
Details
(Keywords: memory-leak, Whiteboard: fixed-in-tracemonkey, softblocker)
Attachments
(3 files)
81.13 KB,
image/png
|
Details | |
6.19 KB,
patch
|
Details | Diff | Splinter Review | |
16.13 KB,
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110115 Firefox/4.0b10pre Build Identifier: 20110115030345 Looks like String.match is leaking or not freeing memory. I've two functions, which return "true" if a regular expresion is found on a String. The first function "LeakingMatch" use String.match The second function "NONLeakingMatch" use a new RegExp When I execute the same thing on "LeakingMatch" memory increase and never down. When I execute the same thing on "NONLeakingMatch" memory increase and after some seconds memory back to normal state. Reproducible: Always Steps to Reproduce: //matchs a regular expresion function NONLeakingMatch(aString, aREGEXP) { if(aREGEXP=='') { delete aString, aREGEXP; return false; } try { aREGEXP = new RegExp(aREGEXP, 'i'); if(aREGEXP.test(aString)) { delete aString, aREGEXP; return true; } else { delete aString, aREGEXP; return false; } } catch(e) { delete aString, aREGEXP, e; return false; } }; //matchs a regular expresion function LeakingMatch (aString, aREGEXP) { if(aREGEXP=='') { delete aString, aREGEXP; return false; } try { var aMatch = aString.match(aREGEXP, 'i'); if(aMatch) { delete aString, aREGEXP, aMatch; return true; } else { delete aString, aREGEXP, aMatch; return false; } } catch(e) { delete aString, aREGEXP, e; return false; } }; /* to reproduce first execute the NONLeaking function to see that memory increases but after a seconds goes back to normal. */ var aRegExp = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; var aString = 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'; var iterations = 500000; //execute this to see expected result for(var i=0;i<iterations;i++) { NONLeakingMatch(aString, aRegExp); } /* to reproduce the leaking execute this a see that memory never returns. On many executions memory just increase and never returns. //execute this to see the bug for(var i=0;i<iterations;i++) { LeakingMatch (aString, aRegExp); } */
Reporter | ||
Comment 1•13 years ago
|
||
Reporter | ||
Comment 2•13 years ago
|
||
Firefox 4 or Minefield.
Updated•13 years ago
|
Assignee: nobody → general
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Component: String → JavaScript Engine
Ever confirmed: true
Keywords: mlk
QA Contact: string → general
Comment 3•13 years ago
|
||
Ouch, great find Roberto! (For 5K iterations) valgrind fingers the leak at: 181,940 (179,964 direct, 1,976 indirect) bytes in 4,999 blocks are definitely lost in loss record 5 of 5 at malloc by js_malloc (jsutil.h:209) by JSRuntime::malloc (jscntxt.h:1404) by JSContext::malloc (jscntxt.h:2032) by js::RegExp::create (jsregexpinlines.h:404) by js::RegExp::createFlagged (jsregexp.cpp:293) by RegExpGuard::normalizeRegExp (jsstr.cpp:1736) by str_match (jsstr.cpp:1866)
Assignee | ||
Comment 4•13 years ago
|
||
Stupid mistake, easy fix.
Assignee: general → cdleary
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•13 years ago
|
||
Reflects the possibility of js::RegExp ownership in the RegExpPair.
Attachment #504295 -
Flags: review?(lw)
Comment 7•13 years ago
|
||
Wow, you already posted a patch while I was at the park! I had been thinking about preventing this type of error and making things more explicit along the line of already_AddRefed that's used in the rest of mozilla.
Attachment #504316 -
Flags: review?(cdleary)
Assignee | ||
Comment 8•13 years ago
|
||
Comment on attachment 504316 [details] [diff] [review] type-y fix Great stuff -- categorically preventing a type of error > fixing one such error! (I probably should have thought to do this.) 1. + typedef RefCountable *****ConvertibleToBool; + bool null() const { return obj == NULL; } + operator ConvertibleToBool() const { return (ConvertibleToBool)obj; } I can see how this may be better than an |operator!() const| (i.e. if you want to test the positive truthiness of this value without using the |!!| idiom), but why that many pointer indirections? Also, is |null() const| necessary? It doesn't appear to be used. 2. Turning the address-of operator into a gimme-a-reference operator doesn't seem right to me -- is there some precedent for doing this? + RefCountable &operator&() const { return *obj; } 3. One thing I did in the other patch was remove the re_ member from this structure -- I think it's a nice simplification to use the value provided through |arc|. - explicit RegExpPair(JSContext *cx, RegExp *re) : arc(cx, re), re_(re) {} + explicit RegExpPair(JSContext *cx) : arc(cx), re_(NULL) {} 4. A comment as to why this is necessary? It wasn't needed before and I don't see any changes to JSContext::create methods. + template <class T, class P1, class P2, class P3> + friend T *JSContext::create(const P1 &, const P2 &, const P3 &); + 5. Our refcount helper templates may want to be moved to jstl.h -- note sure if the passing of cx in the AutoRefCount is enough to keep them in jscntxt.h
Attachment #504316 -
Flags: review?(cdleary) → review+
Assignee | ||
Updated•13 years ago
|
Attachment #504295 -
Flags: review?(lw)
Comment 9•13 years ago
|
||
(In reply to comment #8) > I can see how this may be better than an |operator!() const| (i.e. if you want > to test the positive truthiness of this value without using the |!!| idiom), > but why that many pointer indirections? Also, is |null() const| necessary? It > doesn't appear to be used. The ConvertibleToBool type just needs to be convertible to bool (its in the name). The reason for making it a wacky pointer type is so that it won't accidentally be useful. operator void*()/operator bool() achieve this purpose, but, because of wonky implicit conversion rules, they let you write (a == b) where a and b are two unrelated types. Embedding T in the ConvertibleToBool type ensures that only related types are comparable and gives the expected result. > 2. Turning the address-of operator into a gimme-a-reference operator doesn't > seem right to me -- is there some precedent for doing this? > > + RefCountable &operator&() const { return *obj; } Whoa, slip o' the fingers; I meant operator*(). Thanks! > - explicit RegExpPair(JSContext *cx, RegExp *re) : arc(cx, re), re_(re) {} > + explicit RegExpPair(JSContext *cx) : arc(cx), re_(NULL) {} > > 4. A comment as to why this is necessary? It wasn't needed before and I don't > see any changes to JSContext::create methods. I replaced a malloc/new with cx->create and the constructor is private. Will comment.
Comment 10•13 years ago
|
||
(In reply to comment #8) > Also, is |null() const| necessary? It doesn't appear to be used. Oops, meant to assert(!null()) in operator->() et al; will do now. In general, its nice to give a named version of your operator overloads so that you don't force people to work around operator overloading quirks (e.g., by using !!). > 3. One thing I did in the other patch was remove the re_ member from this > structure -- I think it's a nice simplification to use the value provided > through |arc|. Good observation, will do. > 5. Our refcount helper templates may want to be moved to jstl.h -- note sure if > the passing of cx in the AutoRefCount is enough to keep them in jscntxt.h I think the JSContext does want it out of the "pure" generic stuff in jstl.h. Perhaps later there will be a non-JSContext situation and we will want to parameterize and lift.
Comment 11•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/3084bf4eaa3b
Whiteboard: fixed-in-tracemonkey
Updated•13 years ago
|
blocking2.0: ? → betaN+
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey, softblocker
Assignee | ||
Comment 12•13 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/3084bf4eaa3b
Assignee | ||
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•