468176 - Crash [@ nsAttrValue::ToString] with binding, menulist, observes, etc

Status: VERIFIED FIXED

(Core :: XUL, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See zipped up testcase, which crashes current trunk build after 100ms.
You need to open the file named '_crash1_383.xul'.

This doesn't crash Firefox 3, I can look for a regression range, if wanted.

http://crash-stats.mozilla.com/report/index/701e4932-d25c-463b-b4e2-ff2da2081205?p=1
0  	xul.dll  	nsAttrValue::ToString  	 content/base/src/nsAttrValue.cpp:325
1 	xul.dll 	nsXULElement::UnsetAttr 	content/xul/content/src/nsXULElement.cpp:1284

Comment 1

[Daniel Holbert [:dholbert]]

Cautiously marking this bug as [sg:critical?] and security-sensitive, because of the similarity to the security-sensitive bug 458531 (as martijn pointed out on that bug)

Comment 2

[Daniel Holbert [:dholbert]]

The testcase crashes Linux as well.  Platform --> All/All.

Comment 3

[Daniel Holbert [:dholbert]]

You caught this bug pretty quickly after the regression happened, Martijn. :)  I get this regression range:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20081203 Minefield/3.2a1pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20081204 Minefield/3.2a1pre

Comment 4

[Daniel Holbert [:dholbert]]

Regression range pushlog (with an hour of buffer on either end):
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-12-03+01%3A00&enddate=2008-12-04+03%3A00

Comment 5

[Daniel Holbert [:dholbert]]

Using binary-search on "hg revert --all -r <changeset_id>" with various changesets in the regression range, I've determined that this was caused by one of the three checkins for bug 430214.

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-12-03+03%3A12&enddate=2008-12-03+03%3A14

Comment 6

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

Attachment: recursion limit

...but I'll try something else before asking reviews

Comment 7

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

Comment on attachment 353060 [details] [diff] [review]
recursion limit

I think we need to take something like this, because attribute is set after update, and adding
a binding happens just before that. And adding a binding may do whatever, like removing the attribute.

Comment 8

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

note, document.write has somewhat similar recursion limit (also with max value 20)

Comment 9

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

Comment on attachment 353060 [details] [diff] [review]
recursion limit

Or, I'll try still something else..

Comment 10

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

Attachment: proposed patch

Don't change the attribute when not really needed, but dispatch the
broadcast event whenever attribute has changed on broadcaster.

This fixes also bug 458531.

Comment 11

[Neil Deakin]

Comment on attachment 353555 [details] [diff] [review]
proposed patch

>+                    nsCOMPtr<nsIContent> l = do_QueryInterface(listenerEl);
>+                    if (listenerEl && l) {

Only a check against 'l' is needed as 'listenerEl' would have to be set if 'l' was.

Comment 12

[[e-mail]]

Comment on attachment 353555 [details] [diff] [review]
proposed patch

>+                      // We need to update listener only if we're
>+                      // (1) removing an existing attribute,
>+                      // (2) adding a new attribute or
>+                      // (3) changing the value of an attribute.
>+                      PRBool needsAttrChange =
>+                          (!attrSet && hasAttr) ||
>+                          (attrSet && (!hasAttr || !value.Equals(currentValue)));
attrSet != hasAttr || !value.Equals(currentValue) perhaps?

Comment 13

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

Attachment: +comments

Comment 14

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

Fri Dec 19 14:49:28 2008 +0200 (at Fri Dec 19 14:49:28 2008 +0200)
changeset 22938	187d0de784f2
http://hg.mozilla.org/mozilla-central/rev/187d0de784f2

Comment 15

[Martijn Wargers (dead)]

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

Comment 16

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

Comment on attachment 353696 [details] [diff] [review]
+comments

a191=beltzner

Comment 17

[Reed Loden [:reed]]

Smaug landed this on 1.9.1 earlier -- http://hg.mozilla.org/releases/mozilla-1.9.1/rev/95fa0d1e8913

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on 1.9.1 with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090309 Shiretoko/3.1b4pre ID:20090309020654

Comment 19

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336