FreeBSD 4.1 Mozilla 20010510xx I hit a crash due to view->GetViewManager(viewManager) returning viewManager as 0. Checking in nsFrame.cpp I found that all but 3 references to GetViewManager() checked the result before using it. Attached is a patch to fix those 3. I forget what I was doing when it crashed. It died in nsFrame::Invalidate. I'll also attach a backtrace.

After looking around, I found that about 1/3 of the uses of both presShell->GetViewManager() and view->GetViewManager() don't check the result (if they use nsCOMPtrs and getter_AddRefs() or *getter_AddRefs()), or they don't use nsCOMPtr's and use NS_RELEASE directly. I'm working up a patch, but it hits a LOT of files in layout. Should I keep going, or is the real problem that GetViewManager can fail in the first place? Since 2/3 of the entries _DO_ check for failure, I'm guessing it always can fail (even if it's very unlikely), and so all uses should check for failure. So far the file list is: nsCSSFrameConstructor.cpp nsGfxListControlFrame.cpp nsPresShell.cpp nsViewManager.cpp nsFormControlHelper.cpp nsFrameSetFrame.cpp nsObjectFrame.cpp nsTextFrame.cpp nsLineLayout.cpp nsContainerFrame.cpp nsFrame.cpp inFlasher.cpp but there are a number more.

Also, for cases where there's an assertion on it succeeding - should I let it crash following the assertion (which is debug-only!), or should I make it return an error and hopefully keep running? (I know which one I think I should do, but I'm looking for proper practice hints.)

Looking at nsView.cpp some more - As best I can tell, if we've done nsView::Init(), and didn't get a precondition assertion, then is should NOT be null (unless it has been destroyed! - perhaps that's what caused my original crash.) Some of the methods in nsView check for non-null, others don't. Which is correct?

You know that views are not refcounted, right?

Yes - this is an nsIViewManager, not an nsIView. They do appear to be refcounted (witness all the hand-rolled NS_RELEASE() calls mixed with nsCOMPtr<nsIViewManager> uses).

reassigning to kmcclusk.

If you have a view without a ViewManager something is wrong. This is an error condition. Views will not function properly without a viewmanager. Although some of the code tries to be defensive and checks for a null viewmanager return value it is not intended that views can function without a viewmanager. The frame that holds a reference to a view is responsible for destroying the view. However, if the viewmanager is destroyed before the document's frames are destroyed (which is an error condition) the viewmanager will destroy all of the views. Later, when the frames are destroyed or the frame tries to de-reference the view it will crash. The viewmanager should always be destroyed after the frame manager. ViewManagers are refcounted. Views are not refcounted.

So, then why did we hit this crash? I made a patch to stop it from crashing, but I have no idea why the view had no viewmanager, and this patch does NOT solve that problem. This was the backtrace (edited down): nsFrame::Invalidate nsImageFrame::FrameChanged nsImageListener::FrameChanged imgRequestProxy::FrameChanged imgRequest::FrameChanged imgContainer::Notify So, there's a logic flaw somewhere that either allowed a view to be created without a viewmanager (would have asserted), or allowed it to be deleted while the view still existed (I don't think so), or perhaps allowed the view and the viewmanager to be deleted and the view was actually free memory (I don't think so), or the view was in the middle of being deleted (after mViewManager = nsnull). I should also note that nsView.cpp is full of checks for null mViewManagers.

This bug is a topcrasher, added topcrash keyword. Added [@ nsFrame::Invalidate] to summary for tracking. Here are some URLs & Comments that might help repro this crash: (30688755) URL: www.ditto.com (30688744) URL: www.ditto.com (30687601) URL: http://www.net-temps.com/jpost.htm/prefmi?c=ver02 (30687601) Comments: I closed the second of two open web pages when Mozilla crashed (30623380) Comments: I don't know *what* this is about. The browser was invoked by the program EditPlus. I went to their web site (30623380) Comments: When I read it it said "Unable to invoke default browser." But about as quickly as I could read it the browser crash window opened. And closed. I closed the dialog box (30612862) URL: http://tv.tv.de (30612862) Comments: installed new theme aqua for mozilla 0.9+ (30584423) Comments: Clicked on first search result when VIDEOKAMERA was the search argument. Crashed immediately. (30572331) URL: www.isogen.com Here is a recent stack trace: nsFrame::Invalidate [d:\builds\seamonkey\mozilla\layout\html\base\src\nsFrame.cpp line 2154] nsImageFrame::FrameChanged [d:\builds\seamonkey\mozilla\layout\html\base\src\nsImageFrame.cpp line 427] nsImageFrame::AddRef imgRequestProxy::FrameChanged [d:\builds\seamonkey\mozilla\modules\libpr0n\src\imgRequestProxy.cpp line 262] imgContainer::Notify [d:\builds\seamonkey\mozilla\modules\libpr0n\src\imgContainer.cpp line 440] nsTimer::Fire [d:\builds\seamonkey\mozilla\widget\timer\src\windows\nsTimer.cpp line 190] KERNEL32.DLL + 0x2317 (0xbff62317) $E7 0x13370003 0xfe468308

My patch will stop this precise crash - however, if the view can have a null viewmanager (apparently), it may well end up crashing elsewhere. I do have partially done mods to files all over the system to let them deal with a null viewmanager, however a better fix _may_ be to fix the flaw that allows it to be null. If it makes sense for it to on rare instance be null, I can complete my patch (which is mostly done).

I think that adding a bunch of checks for null view managers in the views or frames is a mistake - views cannot exist without a view manager, so if anything we just need assertions (in fact, those 3 places that are checking for a viewmanager from the view should be changed to assertions, IMO). I think that this bug is related to a problem where a leaking presShell was causing us to have a frame tree with no corresponding viewmanager, and this was fixed by dbaron on Friday - See bug 80203. Is this happening in more recent builds (like, after Friday)? I cannot repro from teh URLs in the talkbacks.

Clearing out keywords, changing title - bug 80203 addresses the crash for which this bug was originally reported. If (as the commentary here and in bug 80203 indicates) views ALWAYS have viewmanagers, then there's lots of pessimized code that silently checks for non-existant viewmanagers that should be changed into assertions. Would this also apply to presshell's? (I'm guessing yes). Also, nsIView.h doesn't mention (though perhaps you can infer) that GetViewManager() always will return a viewmanager.

Downgrading severity since the crash is covered by bug 80203

Resolving WONTFIX.