460444 - Potential crash in ~nsXBLAttributeEntry(), ~nsXBLProtoImplField(), ~nsXBLPrototypeHandler() , ~nsXBLParameter(), ~nsXBLProtoImplMember(), ~nsXBLResource()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mats Palmgren (inactive)]

Like in bug 456196 I suspect this could cause a crash with a long list (mNext):
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xbl/src/nsXBLPrototypeBinding.cpp&rev=1.163&mark=126#126

Comment 1

[Mats Palmgren (inactive)]

Here are a few more to investigate:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/./content/xbl/src/nsXBLProtoImplField.cpp&rev=1.26&root=/cvsroot&mark=74#68

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp&rev=1.142&root=/cvsroot&mark=159#149

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xbl/src/nsXBLProtoImplMethod.h&rev=1.20&root=/cvsroot&mark=62#59

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xbl/src/nsXBLProtoImplMember.h&rev=1.21&root=/cvsroot&mark=102#102

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xbl/src/nsXBLResourceLoader.h&rev=1.10&root=/cvsroot&mark=69#55

Comment 2

[Martijn Wargers (dead)]

Attachment: zipped up testcase, using a lot of images in resources

Yeah, this is crashing. See zipped up testcase. This crashes with this stacktrace:
http://crash-stats.mozilla.com/report/index/d4ec50c7-9c54-11dd-82a8-001cc4e2bf68?p=1
0  	xul.dll  	nsXBLResource::~nsXBLResource  	 content/xbl/src/nsXBLResourceLoader.h:67
1 	xul.dll 	nsXBLResource::~nsXBLResource 	content/xbl/src/nsXBLResourceLoader.h:70
2 	xul.dll 	nsXBLResource::~nsXBLResource 	content/xbl/src/nsXBLResourceLoader.h:70
3 	xul.dll 	nsXBLResource::~nsXBLResource 	content/xbl/src/nsXBLResourceLoader.h:70
4 	xul.dll 	nsXBLResource::~nsXBLResource 	content/xbl/src/nsXBLResourceLoader.h:70
5 	xul.dll 	nsXBLResource::~nsXBLResource 	content/xbl/src/nsXBLResourceLoader.h:70
etc..

Comment 3

[Martijn Wargers (dead)]

I crashed sometimes with a lot of fields, but not reproducable:
http://crash-stats.mozilla.com/report/index/b52f9dee-9c59-11dd-9586-001cc45a2ce4?p=1
0  	mozcrt19.dll  	arena_dalloc_small  	 obj-firefox/memory/jemalloc/src/jemalloc.c:4267
1 	mozcrt19.dll 	arena_dalloc 	obj-firefox/memory/jemalloc/src/jemalloc.c:4390
2 	mozcrt19.dll 	free 	obj-firefox/memory/jemalloc/src/jemalloc.c:6207
3 	xul.dll 	nsXBLProtoImplField::~nsXBLProtoImplField 	content/xbl/src/nsXBLProtoImplField.cpp:73
4 	xul.dll 	nsXBLProtoImplField::~nsXBLProtoImplField 	content/xbl/src/nsXBLProtoImplField.cpp:74
5 	xul.dll 	nsXBLProtoImplField::~nsXBLProtoImplField 	content/xbl/src/nsXBLProtoImplField.cpp:74
6 	xul.dll 	nsXBLProtoImplField::~nsXBLProtoImplField 	content/xbl/src/nsXBLProtoImplField.cpp:74
7 	xul.dll 	nsXBLProtoImplField::~nsXBLProtoImplField 	content/xbl/src/nsXBLProtoImplField.cpp:74
etc..

Comment 4

[Martijn Wargers (dead)]

This one with a lot of constructors:
http://crash-stats.mozilla.com/report/index/225d9cb3-9c5e-11dd-bfdf-0013211cbf8a?p=1
0  	mozcrt19.dll  	arena_dalloc_small  	 obj-firefox/memory/jemalloc/src/jemalloc.c:4257
1 	mozcrt19.dll 	arena_dalloc 	obj-firefox/memory/jemalloc/src/jemalloc.c:4390
2 	mozcrt19.dll 	free 	obj-firefox/memory/jemalloc/src/jemalloc.c:6207
3 	xul.dll 	nsXBLProtoImplAnonymousMethod::`vector deleting destructor' 	
4 	xul.dll 	nsXBLProtoImplAnonymousMethod::`vector deleting destructor' 	
5 	xul.dll 	nsXBLProtoImplAnonymousMethod::`vector deleting destructor' 	
6 	xul.dll 	nsXBLProtoImplAnonymousMethod::`vector deleting destructor' 	
7 	xul.dll 	nsXBLProtoImplAnonymousMethod::`vector deleting destructor' 	
etc..

The crash sometimes occurs when I close the testcase while it is still loading.

Comment 5

[Mats Palmgren (inactive)]

I think we need to fix all of these...  (see also bug 460461)

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 346668 [details] [diff] [review]
Patch rev. 1

r+sr=dbaron, although you probably want a content peer to say that it's ok to have this in nsINode.h

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 346668 [details] [diff] [review]
Patch rev. 1

Looks good to me.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 346668 [details] [diff] [review]
Patch rev. 1

... though you could stick the definition of the macro in nsContentUtils.h rather than nsINode.h.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (moved the #define to nsContentUtils.h)

Moved the #define to nsContentUtils.h, no other significant changes,
I'll land this when the tree opens.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You'll need to request approval1.9? to land after beta 2; see beltzner's dev-planning post on November 13.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348691 [details] [diff] [review]
Patch rev. 2 (moved the #define to nsContentUtils.h)

a191=beltzner

Comment 13

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/e085f7a2037c

No test checked in since it's 2MB big and takes a couple of minutes to load
on a relatively fast PC.

-> FIXED

Comment 14

[Mats Palmgren (inactive)]

Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fe5e09ccc9cc

Comment 15

[Daniel Veditz [:dveditz]]

Do we want to fix this on the 1.9.0 branch?