Closed Bug 395628 Opened 12 years ago Closed 12 years ago

"ASSERTION: post-reflow queues not empty" with feed in <frame>

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect, P2, major)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: jruderman, Assigned: mats)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, memory-leak, testcase, Whiteboard: [dbaron-1.9:Rm])

Attachments

(4 files, 3 obsolete files)

Attached file testcase 1
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.
Flags: blocking1.9?
Assignee: nobody → mats.palmgren
OS: Mac OS X → All
Hardware: PC → All
Attached file stack
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)
Keywords: mlk
Attached patch Patch rev. 1 (obsolete) — Splinter Review
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.
Attachment #280360 - Flags: review?(Olli.Pettay)
As an optimisation, we could also allocate the nsASyncMenuInitialization
and nsAsyncAccesskeyUpdate objects from the pres shell arena.
I'll file that separately.
Group: security
(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.
Attachment #280360 - Flags: review?(Olli.Pettay) → review+
(In reply to comment #6)
> force all the
> implementations to do the right thing.
> 

Or not force, but strongly suggest... :)
Attached patch WIP, alternative fix (obsolete) — Splinter Review
(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()).
Attached patch Patch rev. 2 (obsolete) — Splinter Review
Patch rev. 1 + the nit in comment 6 fixed
Attachment #280360 - Attachment is obsolete: true
Attachment #280410 - Flags: superreview?(dbaron)
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?
Flags: blocking1.9? → blocking1.9+
(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.
Attachment #280410 - Flags: superreview?(dbaron) → superreview+
Whiteboard: [dbaron-1.9:Rm]
Mats, is this patch ready to be checked in?
Priority: P3 → P2
Attached patch Patch rev. 3Splinter Review
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
Attachment #280408 - Attachment is obsolete: true
Attachment #280410 - Attachment is obsolete: true
Attachment #289768 - Flags: superreview?(dbaron)
Attachment #289768 - Flags: review?(dbaron)
Comment on attachment 289768 [details] [diff] [review]
Patch rev. 3

r+sr=dbaron
Attachment #289768 - Flags: superreview?(dbaron)
Attachment #289768 - Flags: superreview+
Attachment #289768 - Flags: review?(dbaron)
Attachment #289768 - Flags: review+
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
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10
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
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.