Loading the testcase triggers: ###!!! ASSERTION: post-reflow queues not empty. This means we're leaking: 'mFirstCallbackEventRequest == nsnull && mLastCallbackEventRequest == nsnull', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 1404 ###!!! ASSERTION: Some objects allocated with AllocateFrame were not freed: 'mFrameCount == 0', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 673 Security-sensitive because the latter assertion often indicates the presence of an exploitable memory safety bug. I haven't tried to determine whether this bug is exploitable.

We have unhandled nsASyncMenuInitialization objects still in the queue when the pres shell is destroyed. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsMenuFrame.cpp&rev=1.355&root=/cvsroot&mark=230-252,303-305#230 These objects doesn't track frame destruction and cancel themselves like some other nsIReflowCallback implementors do, e.g. nsGfxScrollFrameInner: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsGfxScrollFrame.cpp&rev=3.312&root=/cvsroot&mark=1694-1697#1684 The latter strategy costs an extra pointer on the frame object though, which seems like a waste in the nsMenuFrame case since those events have much shorter life than the frame. BTW, nsAsyncAccesskeyUpdate have the same problem: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsTextBoxFrame.cpp&rev=1.119&root=/cvsroot#212 AFAICT, this is not a security problem, but we do leak these objects if the pres shell is destroyed with them in the queue. (ie the shell didn't process any reflow commands after they were queued)

Suggested fix: remove the requirement that nsIReflowCallback implementors remove themselves before the shell is destroyed; instead, make the shell remove them in its Destroy() method and do a Cancel() callback for local object cleanup.

As an optimisation, we could also allocate the nsASyncMenuInitialization and nsAsyncAccesskeyUpdate objects from the pres shell arena. I'll file that separately.

(In reply to comment #2) > BTW, nsAsyncAccesskeyUpdate have the same problem: > http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsTextBoxFrame.cpp&rev=1.119&root=/cvsroot#212 > nsAsyncAccesskeyUpdate should not have the problem because reflow callback is posted during reflow.

Comment on attachment 280360 [details] [diff] [review] Patch rev. 1 I've been thinking Cancel method too. r=me if you make it virtual void Cancel() = 0; That will really force all the implementations to do the right thing.

(In reply to comment #6) > force all the > implementations to do the right thing. > Or not force, but strongly suggest... :)

(In reply to comment #5) > nsAsyncAccesskeyUpdate should not have the problem because reflow callback is > posted during reflow. Right, good point. I think we could actually fix this bug by doing the same for nsASyncMenuInitialization. It's currently queued from Init(), so there is a subtle difference of "do this when *any* reflow batch is done" versus "do this when the reflow batch with *this frame* is done". In practice it will be the same, but I think there is a theoretical possibility that the menu frame has NOT been reflowed when the nsASyncMenuInitialization callback is called with the current strategy (ie when queued from Init()).

Patch rev. 1 + the nit in comment 6 fixed

Comment on attachment 280410 [details] [diff] [review] Patch rev. 2 It seems a bit puzzling that we need this at all. Shouldn't we not be destroying pres shells in the middle of reflow? >Index: layout/base/nsIReflowCallback.h >+ * These are not refcounted. Objects can either be removed from the presshell >+ * callback list before they die or they can do necessary cleanup in the >+ * Cancel() method, which is called on outstanding callback requests when >+ * the shell is destroyed. This comment should be clearer that a reflow callback that is not removed will receive either a ReflowFinished or a Cancel callback -- at least if that's what you're doing here, which I think it is. Cancel seems like an awfully generic name to use for an interface that's implemented on frames, though. Maybe it should be ReflowCallbackCanceled?

Or you're saying we're now using reflow callbacks from places other than inside reflow?

(In reply to comment #11) > Or you're saying we're now using reflow callbacks from places other than inside > reflow? > Yes, in some cases reflow callbacks are posted before the first reflow, IIRC.

Comment on attachment 280410 [details] [diff] [review] Patch rev. 2 OK, sr=dbaron if you rename Cancel (see above comment) and document when reflow callbacks are posted.

Mats, is this patch ready to be checked in?

Changes since rev. 2: 1. s/Cancel/ReflowCallbackCanceled/ 2. clarified the comment in nsIReflowCallback.h (I hope) 3. added ReflowCallbackCanceled() for nsProgressMeterFrame and nsSubDocumentFrame which are new consumers since last patch

Comment on attachment 289768 [details] [diff] [review] Patch rev. 3 r+sr=dbaron

Checking in layout/base/nsIReflowCallback.h; /cvsroot/mozilla/layout/base/nsIReflowCallback.h,v <-- nsIReflowCallback.h new revision: 3.8; previous revision: 3.7 done Checking in layout/base/nsPresShell.cpp; /cvsroot/mozilla/layout/base/nsPresShell.cpp,v <-- nsPresShell.cpp new revision: 3.1079; previous revision: 3.1078 done Checking in layout/generic/nsFrameFrame.cpp; /cvsroot/mozilla/layout/generic/nsFrameFrame.cpp,v <-- nsFrameFrame.cpp new revision: 3.326; previous revision: 3.325 done Checking in layout/generic/nsGfxScrollFrame.cpp; /cvsroot/mozilla/layout/generic/nsGfxScrollFrame.cpp,v <-- nsGfxScrollFrame.cpp new revision: 3.319; previous revision: 3.318 done Checking in layout/generic/nsGfxScrollFrame.h; /cvsroot/mozilla/layout/generic/nsGfxScrollFrame.h,v <-- nsGfxScrollFrame.h new revision: 3.116; previous revision: 3.115 done Checking in layout/xul/base/src/nsListBoxBodyFrame.cpp; /cvsroot/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp,v <-- nsListBoxBodyFrame.cpp new revision: 1.98; previous revision: 1.97 done Checking in layout/xul/base/src/nsListBoxBodyFrame.h; /cvsroot/mozilla/layout/xul/base/src/nsListBoxBodyFrame.h,v <-- nsListBoxBodyFrame.h new revision: 1.30; previous revision: 1.29 done Checking in layout/xul/base/src/nsMenuFrame.cpp; /cvsroot/mozilla/layout/xul/base/src/nsMenuFrame.cpp,v <-- nsMenuFrame.cpp new revision: 1.364; previous revision: 1.363 done Checking in layout/xul/base/src/nsProgressMeterFrame.cpp; /cvsroot/mozilla/layout/xul/base/src/nsProgressMeterFrame.cpp,v <-- nsProgressMeterFrame.cpp new revision: 1.64; previous revision: 1.63 done Checking in layout/xul/base/src/nsTextBoxFrame.cpp; /cvsroot/mozilla/layout/xul/base/src/nsTextBoxFrame.cpp,v <-- nsTextBoxFrame.cpp new revision: 1.124; previous revision: 1.123 done Checking in layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp; /cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v <-- nsTreeBodyFrame.cpp new revision: 1.342; previous revision: 1.341 done Checking in layout/xul/base/src/tree/src/nsTreeBodyFrame.h; /cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.h,v <-- nsTreeBodyFrame.h new revision: 1.122; previous revision: 1.121 done

verified fixed for trunk using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120520 Minefield/3.0b2pre and the testcases from jesse. No assertions on testcase. -> Verified