Last Comment Bug 461053 - [FIX] Crash [@ nsXULDocument::SynchronizeBroadcastListener] with command
: [FIX] Crash [@ nsXULDocument::SynchronizeBroadcastListener] with command
Status: VERIFIED FIXED
[sg:investigate] post 1.8-branch
: crash, testcase, verified1.9.0.9, verified1.9.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
-- critical (vote)
: mozilla1.9.2a1
Assigned To: Olli Pettay [:smaug] (pto-ish for couple of days)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-21 15:26 PDT by Martijn Wargers [:mwargers]
Modified: 2011-06-13 10:01 PDT (History)
11 users (show)
jst: blocking1.9.1-
dveditz: blocking1.9.0.9+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
hskupin: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (594 bytes, application/vnd.mozilla.xul+xml)
2008-10-21 15:27 PDT, Martijn Wargers [:mwargers]
no flags Details
backtrace from trunk crash (3.93 KB, text/plain)
2008-10-23 11:32 PDT, Brandon Sterne (:bsterne)
no flags Details
create a list of to-be-broadcasted attributes before broadcasting (3.37 KB, patch)
2008-11-12 06:20 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
neil: superreview+
Details | Diff | Splinter Review
v2 (3.29 KB, patch)
2008-11-12 13:43 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
enndeakin: review+
neil: superreview+
mbeltzner: approval1.9.1+
dveditz: approval1.9.0.9+
Details | Diff | Splinter Review

Description User image Martijn Wargers [:mwargers] 2008-10-21 15:26:22 PDT
See testcase, which crashes current trunk build and Firefox 3 within 100ms.

http://crash-stats.mozilla.com/report/index/69096ca8-9fbd-11dd-9496-001a4bd43e5c?p=1
0  	xul.dll  	nsXULDocument::SynchronizeBroadcastListener  	 content/xul/document/src/nsXULDocument.cpp:703
1 	xul.dll 	nsTArray_base::SwapArrayElements 	obj-firefox/xpcom/build/nsTArray.cpp:205
2 	xul.dll 	nsXULDocument::EndUpdate 	content/xul/document/src/nsXULDocument.cpp:3271
Comment 1 User image Martijn Wargers [:mwargers] 2008-10-21 15:27:17 PDT
Created attachment 344193 [details]
testcase
Comment 2 User image Brandon Sterne (:bsterne) 2008-10-23 11:32:10 PDT
Created attachment 344512 [details]
backtrace from trunk crash

I'm having trouble determining the exploitability of this bug.  Here's a backtrace from the crash on a fresh Linux m-c debug build.
Comment 3 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2008-11-12 06:20:55 PST
Created attachment 347769 [details] [diff] [review]
create a list of to-be-broadcasted attributes before broadcasting

...and make sure the attribute still there in broadcaster before setting
it to listener.
Comment 4 User image neil@parkwaycc.co.uk 2008-11-12 07:16:51 PST
Comment on attachment 347769 [details] [diff] [review]
create a list of to-be-broadcasted attributes before broadcasting

From whom did you intend review? ;-)
Comment 5 User image Neil Deakin 2008-11-12 11:25:28 PST
Comment on attachment 347769 [details] [diff] [review]
create a list of to-be-broadcasted attributes before broadcasting

>-            const nsAttrName* attrName = broadcaster->GetAttrNameAt(count);
>+        nsTArray<nsAttrNameInfo> attributes(count);
>+        for (PRUint32 i = 0; i < count; ++i) {
>+            const nsAttrName* attrName = broadcaster->GetAttrNameAt(i);
>             PRInt32 nameSpaceID = attrName->NamespaceID();
>             nsIAtom* name = attrName->LocalName();
> 
>             // _Don't_ push the |id|, |ref|, or |persist| attribute's value!
>             if (! CanBroadcast(nameSpaceID, name))
>                 continue;
> 
>+            attributes.AppendElement(nsAttrNameInfo(attrName->NamespaceID(),
>+                                                    attrName->LocalName(),
>+                                                    attrName->GetPrefix()));

You should either use the temporary variables in both cases or neither.
>-                              mInitialLayoutComplete);
>+            if (broadcaster->GetAttr(nameSpaceID, name, value)) {
>+              listener->SetAttr(nameSpaceID, name, attributes[count].mPrefix,
>+                                value, mInitialLayoutComplete);
>+            }

What if the attribute has a value on the listener but not on the broadcaster? Is this creating different behaviour in some situation?
Comment 6 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2008-11-12 13:41:11 PST
(In reply to comment #5)
> You should either use the temporary variables in both cases or neither.
Indeed, a left over from a previous (local) version. Fixing.

> What if the attribute has a value on the listener but not on the broadcaster?
> Is this creating different behaviour in some situation?
This is creating different behavior than currently only if someone 
modifies attribute using mutation listeners.
Note, if the element if removed (which if(GetAttr) checks), other code
should already remove the attribute from listener.
So this is IMO fixing the problem when removed attribute is recreated on
listener with empty value.
Comment 7 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2008-11-12 13:43:46 PST
Created attachment 347850 [details] [diff] [review]
v2
Comment 8 User image Brandon Sterne (:bsterne) 2008-11-18 10:36:21 PST
Looks like we have a reviewed patch here.  Can we get this landed?
Comment 9 User image Martijn Wargers [:mwargers] 2008-11-18 12:30:43 PST
The patch needs sr+, afaik.
Comment 10 User image Martijn Wargers [:mwargers] 2008-11-18 16:44:46 PST
The patch still isn't ready for check-in, afaik.
This bug has to be blocking1.9.1+ or the patch needs to get approval1.9.1 or both.
Comment 11 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2008-11-18 16:46:39 PST
Ah, indeed!
Comment 12 User image Mike Beltzner [:beltzner, not reading bugmail] 2008-11-26 21:16:54 PST
Comment on attachment 347850 [details] [diff] [review]
v2

a191=beltzner
Comment 13 User image Johnny Stenback (:jst, jst@mozilla.com) 2008-12-02 15:04:52 PST
This is ready to go, not marking as a blocker.
Comment 14 User image Martijn Wargers [:mwargers] 2008-12-04 16:19:22 PST
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081204 Minefield/3.2a1pre
Comment 15 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2008-12-06 09:12:19 PST
Fixed on trunk and 1.9.1
Comment 16 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2009-02-04 17:09:25 PST
Verified on 1.9.1 using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre

Martijn, can we add the testcase to the crashtest harness?
Comment 17 User image Daniel Veditz [:dveditz] 2009-02-12 16:00:08 PST
This is always a null deref in debug builds, but could it be uninitialized in optimized builds? Can't tell, let's just make sure it's fixed on 1.9.0
Comment 18 User image Daniel Veditz [:dveditz] 2009-03-16 15:24:44 PDT
Olli: does this patch work for the 1.9.0 branch or will it require porting?
Comment 19 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-03-17 03:13:26 PDT
Comment on attachment 347850 [details] [diff] [review]
v2

Applies to 1.9.0 with some small fuzz
Comment 20 User image Daniel Veditz [:dveditz] 2009-03-17 12:19:39 PDT
Comment on attachment 347850 [details] [diff] [review]
v2

Approved for 1.9.0.8, a=dveditz
Comment 21 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2009-03-17 12:54:57 PDT
Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.831; previous revision: 1.830
done
Comment 22 User image Al Billings [:abillings] 2009-03-19 16:43:27 PDT
Verified for 1.9.0.8 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8pre) Gecko/2009031904 GranParadiso/3.0.8pre. Crashes cleanly in 1.9.0.7.

Note You need to log in before you can comment on or make changes to this bug.