Bugzilla@Mozilla – Bug 461053
[FIX] Crash [@ nsXULDocument::SynchronizeBroadcastListener] with command
Last modified: 2009-04-21 14:08:07 PDT
Summon comment box
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
Created attachment 344193 [details] testcase
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.
Created attachment 347769 [details] [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 on attachment 347769 [details] [review] create a list of to-be-broadcasted attributes before broadcasting From whom did you intend review? ;-)
Comment on attachment 347769 [details] [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?
(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.
Created attachment 347850 [details] [review] v2
Looks like we have a reviewed patch here. Can we get this landed?
The patch needs sr+, afaik.
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.
Ah, indeed!
Comment on attachment 347850 [details] [review] v2 a191=beltzner
This is ready to go, not marking as a blocker.
Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081204 Minefield/3.2a1pre
Fixed on trunk and 1.9.1
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?
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
Olli: does this patch work for the 1.9.0 branch or will it require porting?
Comment on attachment 347850 [details] [review] v2 Applies to 1.9.0 with some small fuzz
Comment on attachment 347850 [details] [review] v2 Approved for 1.9.0.8, a=dveditz
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
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.