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

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: 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.

Comment 1

[Jesse Ruderman]

Attachment: testcase 2 (triggers the assertions after two seconds)

Comment 2

[Mats Palmgren (inactive)]

Attachment: 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)

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

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.

Comment 4

[Mats Palmgren (inactive)]

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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(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 6

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #6)
> force all the
> implementations to do the right thing.
> 

Or not force, but strongly suggest... :)

Comment 8

[Mats Palmgren (inactive)]

Attachment: WIP, alternative fix

(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()).

Comment 9

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Patch rev. 1 + the nit in comment 6 fixed

Comment 10

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

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?

Comment 11

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

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

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(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 13

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

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.

Comment 14

[Jesse Ruderman]

Mats, is this patch ready to be checked in?

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

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 16

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

Comment on attachment 289768 [details] [diff] [review]
Patch rev. 3

r+sr=dbaron

Comment 17

[Reed Loden [:reed]]

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

Comment 18

[Carsten Book [:Tomcat]]

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