Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Description • 17 years ago

libGMalloc is awesome at catching mem corruption bugs on mac, and I can't run firefox under it anymore. It dies here on startup:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xd91f0a44
0x007d2f5c in nsCharTraits<unsigned short>::copy (s1=0xbfffe5a0, s2=0xd91f0a44, n=10) at nsCharTraits.h:224

Stack trace attached.

	John Daggett (:jtd) Assignee
	Comment 1 • 17 years ago

Grabbing this for investigation.  Scream if you're already working on it.

	Damon Sicore (:damons)
	Comment 2 • 17 years ago

Wouldn't hold the release for this.  If this becomes an issue, please re-nom.

	John Daggett (:jtd) Assignee
	Comment 3 • 17 years ago

This appears to be a different variation of bug 427941 related to the use of nsTArray<nsAutoString>.  In this case, one of these is used in gfxQuartzFontCache::InitBadUnderlineList with an initial size of 10 elements.  The number of blacklisted fonts is 22, so resizing occurs when the 11th element is inserted and this is when libgmalloc squawks.  As a temporary work around, bump up the size of this array to 30 and everything works fine.  Don't know how anyone uses libgmalloc though, it takes ages for any operation to occur.

	John Daggett (:jtd) Assignee
	Comment 4 • 17 years ago

Note on usage, to enable libgmalloc use:

export DYLD_INSERT_LIBRARIES=/usr/lib/libgmalloc.dylib

	Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Comment 6 • 17 years ago

(In reply to comment #3)
> related to the use of nsTArray<nsAutoString>

John meant nsAutoTArray<nsAutoString>

	timeless
	Comment 8 • 17 years ago

failing to copy over dependencies and descriptions results in bad triage. :(

	John Daggett (:jtd) Assignee
	Comment 9 • 17 years ago

Since this has the potential to cause crashes, we should probably block on this.

	John Daggett (:jtd) Assignee
	Comment 10 • 17 years ago

switch instances of nsTArray<nsAutoString> to nsTArray<nsString> within gfx code.  This includes changes for both Mac and Windows.

	John Daggett (:jtd) Assignee
	Comment 11 • 17 years ago

argh, remove debugging code...

	Mike Schroepfer
	Comment 12 • 17 years ago

(In reply to comment #10)
> Created an attachment (id=315483) [details]
> patch, v.0.1, remove use of nsTArray<nsAutoString>
> 
> switch instances of nsTArray<nsAutoString> to nsTArray<nsString> within gfx
> code.  This includes changes for both Mac and Windows.
> 

Can you give a quick explanation of what this change does?  Is this a real issue exposed by libGMalloc or just an issue specific with libGMalloc? 

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 13 • 17 years ago

(In reply to comment #9)
> Since this has the potential to cause crashes, we should probably block on
> this.

But only for people running under libgMalloc, right? If so, then it still doesn't block. If I'm misinterpreting, please renom. Also, feel free to nom a patch for approval.

	Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Comment 14 • 17 years ago

libGMalloc is a debug library designed to expose memory corruption and incorrect memory usage bugs. The crash is a "feature" that tells us we have a real problem.

This is a real bug that may or may not crash in release builds depending on how the code gets optimized. We should definitely fix this, especially since we know what's wrong and how to make it work correctly.

	John Daggett (:jtd) Assignee
	Comment 15 • 17 years ago

(In reply to comment #13)
> But only for people running under libgMalloc, right? If so, then it still
> doesn't block. If I'm misinterpreting, please renom. Also, feel free to nom a
> patch for approval.

Running with libgmalloc forces stricter memory checking than normally, so if there is any attempt by code to touch memory that is not already explicitly allocated will cause an exception.  It is essentially enforcing rules that should normally be obeyed.  As Jesse put in the description for bug 427941, the problem here is the use of nsAutoTArray<nsAutoString>: 

https://bugzilla.mozilla.org/show_bug.cgi?id=427941#c0

> * nsAutoTArray uses memcpy when it expands.
> * nsAutoString contains an internal pointer to its built-in buffer.
> * Moving using memcpy causes internal pointers to point to the wrong place.

See Jonas' comment from the same bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=427941#c3

> And also add a BIG comment saying that usually putting nsAutoStrings 
> inside an nsTArray is unsafe, but since we do the SetCapacity trick 
> it's ok in this instance. We wouldn't want people to repeat my 
> mistake here.

Frankly, seems to me this usage *should* work but given that the mechanics means that it doesn't, it's safer to change our code to use nsString rather than make changes to a string class at this late stage.

With bug 427941 this usage causes a crash.  Using libgmalloc helps find situations where the code is wrong but we're not necessarily crashing due to luck.

We need to fix this.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 16 • 17 years ago

I don't know what you mean by "should work". You mean we should make nsTArray use copy construction instead of memcpy in general?

It's also unsafe to use nsTArray<nsAutoTArray<>>, and that burned me once.

I agree this should be fixed for 1.9. The fix is really safe.

We should probably make nsTArray safe by using copy construction by default (and optimizing to memcpy in certain cases).

	Mike Schroepfer
	Comment 17 • 17 years ago

Due to Comment 14, Comment 15, and Comment 16 (thanks folks - very helpful) moving this to the blocking list.

	John Daggett (:jtd) Assignee
	Comment 18 • 17 years ago

(In reply to comment #16)
> I don't know what you mean by "should work". You mean we should make nsTArray
> use copy construction instead of memcpy in general?

Ideally either it should work or it shouldn't compile.  Having hidden patterns of use in core code like this is deadly.  The usage within GFX/Thebes was taken from code that Stuart wrote originally and he's been working on the project for a long time, if it's something he can miss this than I'm sure others starting work on Gecko can do the same.

	Jesse Ruderman
	Comment 19 • 17 years ago

That discussion might be more on-topic in bug 428465.

	Stuart Parmenter
	Comment 20 • 17 years ago

Comment on attachment 315484 [details] [diff] [review]
patch, v.0.2, remove use of nsTArray<nsAutoString> (cleanup)

thanks for putting this together ben!  We should revist this later, perhaps when we can do copy constructors rather than memcpys, or we can get rid of the internal pointer of nsAutoStrings.

	Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Comment 21 • 17 years ago

(In reply to comment #20)
> thanks for putting this together ben!

Nah, this was all john :)

	Mike Schroepfer
	Comment 22 • 17 years ago

Comment on attachment 315484 [details] [diff] [review]
patch, v.0.2, remove use of nsTArray<nsAutoString> (cleanup)

Let's get this in..

	John Daggett (:jtd) Assignee
	Comment 23 • 17 years ago

checked in

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 24 • 17 years ago

I backed this out to try to fix test failures.

	Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Comment 25 • 17 years ago

Was this bug's patch the culprit?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 26 • 17 years ago

No. Please reland.

	John Daggett (:jtd) Assignee
	Comment 27 • 17 years ago

(In reply to comment #26)
> No. Please reland.

Will reland during passing moments of tree non-crispiness...