Closed Bug 626118 Opened 13 years ago Closed 13 years ago

String.match leaking

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
major

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)

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);
}
*/
Firefox 4 or Minefield.
Assignee: nobody → general
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Component: String → JavaScript Engine
Ever confirmed: true
Keywords: mlk
QA Contact: string → general
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)
Stupid mistake, easy fix.
Assignee: general → cdleary
Status: NEW → ASSIGNED
Attached patch Fix regexp leak.Splinter Review
Reflects the possibility of js::RegExp ownership in the RegExpPair.
Attachment #504295 - Flags: review?(lw)
Suspect this is related to bug 625480.

/be
Blocks: 625480
Attached patch type-y fixSplinter Review
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)
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+
Attachment #504295 - Flags: review?(lw)
(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.
(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.
blocking2.0: ? → betaN+
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey, softblocker
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.

Attachment

General

Created:
Updated:
Size: