192130 - [FIXr]crashes [@ nsXULDocument::ContentAppended]

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

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

There have been crashes at nsXULDocument::ContentAppended recently.  The date
distribution and the location of the crash makes me suspect a regression from
bug 189384.  The date distribution is:

      4 2003013104
      1 2003020208
      6 2003020308
      3 2003020408
      1 2003020508
      1 2003020511

The crash looks like it's a crash dereferencing |observer| in the following loop
at the very end of nsXULDocument::ContentAppended:

    // Now notify external observers
    PRInt32 count = mObservers.Count();
    // XXXdwh There is a hacky ordering dependency between the binding manager
    // and the frame constructor that forces us to walk the observer list
    // in a forward order
    for (PRInt32 i = 0; i < count; i++) {
        nsIDocumentObserver*  observer = (nsIDocumentObserver*)mObservers[i];
        observer->ContentAppended(this, aContainer, aNewIndexInContainer);
    }

Note that this loop goes in forward order so it's not tolerant of observer
removal during the loop.

The example stack I have is:

         nsXULDocument::ContentAppended
[c:/builds/seamonkey/mozilla/content/xul/document/src/nsXULDocument.cpp  line 2212]
         nsXULElement::AppendChildTo   
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp  line 2232]
         nsXULElement::InsertBefore    
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp  line 1034]
         nsDocument::AppendChild       
[c:/builds/seamonkey/mozilla/content/base/src/nsDocument.cpp  line 3295]
         XPTC_InvokeByIndex    
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp 
line 102]
         XPCWrappedNative::CallMethod  
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp  line 2025]
         XPC_WN_CallMethod     
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp 
line 1293]         js_Invoke      [c:/builds/seamonkey/mozilla/js/src/jsinterp.c
 line 841]
         js_Interpret   [c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 2804]
         js_Invoke      [c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 857]
         js_InternalInvoke      [c:/builds/seamonkey/mozilla/js/src/jsinterp.c 
line 932]
         JS_CallFunctionValue   [c:/builds/seamonkey/mozilla/js/src/jsapi.c 
line 3433]
         nsJSContext::CallEventHandler 
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp  line 1043]
         nsJSEventListener::HandleEvent
[c:/builds/seamonkey/mozilla/dom/src/events/nsJSEventListener.cpp  line 183]
         nsXBLPrototypeHandler::ExecuteHandler 
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp
 line 455]
         nsXBLPrototypeHandler::BindingAttached
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp
 line 543]
         nsXBLPrototypeBinding::BindingAttached
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLPrototypeBinding.cpp
 line 440]
         nsXBLBinding::ExecuteAttachedHandler  
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLBinding.cpp  line 1045]
         nsXBLBinding::ExecuteAttachedHandler  
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLBinding.cpp  line 1042]
         nsBindingManager::ProcessAttachedQueue
[c:/builds/seamonkey/mozilla/content/xbl/src/nsBindingManager.cpp  line 985]
         nsCSSFrameConstructor::ContentInserted
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp 
line 9297]
         StyleSetImpl::ContentInserted 
[c:/builds/seamonkey/mozilla/content/base/src/nsStyleSet.cpp  line 1594]
         PresShell::ContentInserted    
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp  line 5195]
         nsXBLResourceLoader::NotifyBoundElements      
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLResourceLoader.cpp  line 287]
         nsXBLResourceLoader::StyleSheetLoaded 
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLResourceLoader.cpp  line 202]
         CSSLoaderImpl::SheetComplete  
[c:/builds/seamonkey/mozilla/content/html/style/src/nsCSSLoader.cpp  line 1771]
        CSSLoaderImpl::ParseSheet     
[c:/builds/seamonkey/mozilla/content/html/style/src/nsCSSLoader.cpp  line 1715]
        SheetLoadData::OnStreamComplete       
[c:/builds/seamonkey/mozilla/content/html/style/src/nsCSSLoader.cpp  line 1115]
         nsUnicharStreamLoader::OnStopRequest  
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsUnicharStreamLoader.cpp  line 189]
         nsJARChannel::OnStopRequest   
[c:/builds/seamonkey/mozilla/netwerk/protocol/jar/src/nsJARChannel.cpp  line 653]
         nsCOMPtr_base::assign_with_AddRef     
[c:/builds/seamonkey/mozilla/xpcom/glue/nsCOMPtr.cpp  line 70]
         nsInputStreamPump::OnStateStop
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsInputStreamPump.cpp  line 462]
         nsInputStreamPump::OnInputStreamReady 
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsInputStreamPump.cpp  line 321]
         nsInputStreamReadyEvent::EventHandler 
[c:/builds/seamonkey/mozilla/xpcom/io/nsStreamUtils.cpp  line 102]
         PL_HandleEvent [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c 
line 664]
         PL_ProcessPendingEvents       
[c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c  line 597]
         nsEventQueueImpl::ProcessPendingEvents
[c:/builds/seamonkey/mozilla/xpcom/threads/nsEventQueue.cpp  line 391]

User comments aren't very enlightening:
     (16987850) Comments: I was starting Mozilla after installing Calendar.
     (16961014) Comments: I had pushed Ctrl T to create new tab.   Had a number
of tabs open  hadn't touched it since last night.  Each tab just had HTML...
     (16945526) Comments: Going to Tools &gt; Navigator while IMAP was
downloading messages
     (16899544) Comments: Crashed when bookmark was selected from bookmark menu.
     (16822270) Comments: closed mozilla to get rid of tabs in window  restarted
mozilla  was moving the mouse in the lower right corner   seeing some tooltips.
 opened www.heise.de from one of many folders with bookmarks in my personal
toolbar  opened new tab  wanted to open
     (16822270) Comments:  another newsticker  from same folder of bookmarks 
but crashed  before I select it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Sync up observer stuff between nsDocument and nsXULDocument a bit

Dunno if that fixes this crash, but it should be done anyway.

Comment 2

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

Comment on attachment 113699 [details] [diff] [review]
Sync up observer stuff between nsDocument and nsXULDocument a bit

r=dbaron, although this patch is inconsistent about |--i| vs. |i--| (and same
for ++).  I prefer |--i|.  See bug 78032 comment 1.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 113699 [details] [diff] [review]
Sync up observer stuff between nsDocument and nsXULDocument a bit

sr=jst

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113699 [details] [diff] [review]
Sync up observer stuff between nsDocument and nsXULDocument a bit

Could this be approved for 1.3b?  The main idea here is just to rearrange some
code to guard against self-removing observers....

Comment 5

[Asa Dotzler [:asa]]

Comment on attachment 113699 [details] [diff] [review]
Sync up observer stuff between nsDocument and nsXULDocument a bit

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I can't check this in today (no chance to watch the tree).  Could someone land
this, please?

Comment 7

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

Comment on attachment 113699 [details] [diff] [review]
Sync up observer stuff between nsDocument and nsXULDocument a bit

Checked in to trunk, 2003-02-06 19:15 PST.

Comment 8

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

(Although I'm tempted to back out the nsDocument half, considering where we are
relative to 1.3b...)

Comment 9

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

I backed out the nsDocument half of the changes.  I was nervous about them even
before I discovered they make http://dbaron.org/home crash.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: patch for nsDocument only

Yeah... that explains a crash I was seeing this morning on another testcase,
only in the opt build with this patch (debug worked fine....).

The problem is this change in nsDocument::ContentStatesChanged:

-  for (i = 0; i < mObservers.Count(); i++) {
+  for (i = mObservers.Count(); i >= 0; --i) {

Changing that fixes the crash I was seeing, at least.  ;)

Here's a patch for nsDocument that fixes that; I'd still like to land those
changes in the 1.4a or so timeframe, if only because of the binding manager
wackiness...

David, thanks a ton for checking that in (and especially for backing that
nsDocument part out).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113774 [details] [diff] [review]
patch for nsDocument only

Same thing, pretty much, with the one change I pointed out. 

This time I'll be running with it till 1.4a, so it's getting lots of testing...
;)

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 113774 [details] [diff] [review]
patch for nsDocument only

sr=jst

Too bad that mistake wasn't caught by either of us reviewers, it's unfortunate
that it's so hard to catch small differences in repetetive changes...

Comment 13

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

Comment on attachment 113774 [details] [diff] [review]
patch for nsDocument only

r=dbaron, but this part is 1.4alpha material anyway because there could be
ordering issues in code other than the binding manager.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in the nsDocument changes for 1.4a.