470687 - Crash [@ PR_vsnprintf] with observes, mathml element and setting color attrbute

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build after 100ms.
It doesn't crash in a 2008-12-03 build, it does crash in a 2008-12-04 build:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-12-03+04%3A00%3A00&enddate=2008-12-04+06%3A00%3A00

http://crash-stats.mozilla.com/report/index/9918f626-058b-4243-8f9b-4606c2081221?p=1
0  	nspr4.dll  	PR_vsnprintf  	 nsprpub/pr/src/io/prprf.c:1184
1 	nspr4.dll 	PR_snprintf 	nsprpub/pr/src/io/prprf.c:1164
2 	xul.dll 	NS_RGBToHex 	gfx/src/nsColor.cpp:208
3 	xul.dll 	xul.dll@0x2c0f42 	

Unminimized testcases used to crash with this breakpad stack, btw:
http://crash-stats.mozilla.com/report/index/dc1e727d-4fd2-4e1c-b6e6-8f4db2081220?p=1
0  	xul.dll  	nsAttrValue::ToString  	 content/base/src/nsAttrValue.cpp:325
1 	xul.dll 	nsXULDocument::AttributeChanged 	content/xul/document/src/nsXULDocument.cpp:1011
2 	xul.dll 	nsGenericDOMDataNode::cycleCollection::UnmarkPurple

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Fun. Apparently this happens because in some cases color attribute is serialized
to #000, other cases to #000000.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a testcase with mutation listener

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase 2 with a mutation listener

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible fix - trying to detect possible loop

Mutation events and broadcasters are so fun :/

The patch removes temporary delayedAttrChangeBroadcasts and when
the actual broadcasting happens, mDelayedAttrChangeBroadcasts is iterated
until the end of it (even if new items are appended to it).
mHandlingDelayedAttrChange makes sure that nested SetAttr calls don't restart 
iteration.

The cycle detection happens in nsXULDocument::AttributeChanged before anything 
is appended to mDelayedAttrChangeBroadcasts.

Fixes also bug 471416.

Comment 5

[Neil Deakin]

Comment on attachment 354895 [details] [diff] [review]
possible fix - trying to detect possible loop

OK, but the other Neil should probably give a superreview.

Comment 6

[[e-mail]]

Comment on attachment 354895 [details] [diff] [review]
possible fix - trying to detect possible loop

>+                        PRBool possibleCycle = PR_FALSE;
>+                        for (PRUint32 j = 0; j < mDelayedAttrChangeBroadcasts.Length(); ++j) {
>+                            if (mDelayedAttrChangeBroadcasts[j].mListener == listenerEl &&
>+                                mDelayedAttrChangeBroadcasts[j].mAttrName == aAttribute) {
>+                                possibleCycle = PR_TRUE;
>+                                break;
>+                            }
>+                        }
I'm sure it must be possible to use nsTArray::Contains somehow.

>+                        if (possibleCycle) {
>+                            NS_WARNING("Broadcasting loop!");
>+                        } else {
I'd prefer separate NS_WARN_IF_FALSE(!possibleCycle) and if (!possibleCycle)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 354895 [details] [diff] [review]
possible fix - trying to detect possible loop

We need this on 1.9.1 too :/

Comment 8

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090108 Minefield/3.2a1pre

Comment 9

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

Comment on attachment 354895 [details] [diff] [review]
possible fix - trying to detect possible loop

a191=beltzner

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1a72e7f44b09

Comment 11

[Aakash Desai [:aakashd]]

verified FIXED on Shiretoko: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre ID:20090422042031