374468 - Crash [@ PresShell::FrameNeedsReflow] with position: fixed, overflow:scroll and ::before text

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P3)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, you need to download the testcase to your computer, because of the use of enhanced privileges.
It doesn't always crash directly, try reloading a few times to get the crash.

Talkback ID: TB30381350Q
0x00000000
PresShell::FrameNeedsReflow  [mozilla/layout/base/nspresshell.cpp, line 3310]
nsBlockFrame::RemoveFrame  [mozilla/layout/generic/nsblockframe.cpp, line 4890]
nsFrameManager::RemoveFrame  [mozilla/layout/base/nsframemanager.cpp, line 711]
nsCSSFrameConstructor::RemoveFixedItems  [mozilla/layout/base/nscssframeconstructor.cpp, line 12834]
nsCSSFrameConstructor::ReconstructDocElementHierarchyInternal  [mozilla/layout/base/nscssframeconstructor.cpp, line 7657]

Comment 1

[Martijn Wargers (dead)]

Still crashing, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070417 Minefield/3.0a4pre

Comment 2

[Martijn Wargers (dead)]

Doesn't seem to crash anymore, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070512 Minefield/3.0a5pre
But I don't see the text input anymore while loading the testcase. I think it should be shown. I guess that might be the actual reason that it doesn't crash anymore.

Comment 3

[Martijn Wargers (dead)]

Never mind, still crashing, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070524 Minefield/3.0a5pre
You need to use "save link as" to save the testcase to your computer, then load the testcase from your computer to get the crash.

Comment 4

[Boris Zbarsky [:bzbarsky]]

This seems to work for me, even locally...

Comment 5

[Daniel Holbert [:dholbert]]

I got the crash, using a 6/28 build.   Make sure you use "save link as" to save the test case, like Martijn said.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That's exactly what I did...  Are we passing a deleted frame in or something?  Or is one of the other frames in the frame tree dead?

Comment 7

[Daniel Holbert [:dholbert]]

It seems to be random whether the text box appears or doesn't appear.  Here are the assertions I get, in each case.  (In both of these cases, I didn't actually get the crash)

No Text Box Case:
-----------------
###!!! ASSERTION: Attempt to insert row into wrong map.: 'Error', file /scratch/work/builds/trunk.07-06-28.16-46/mozilla/layout/tables/nsCellMap.cpp, line 596
###!!! ASSERTION: Already have an undisplayed context entry for aContent: '!GetUndisplayedContent(aContent)', file /scratch/work/builds/trunk.07-06-28.16-46/mozilla/layout/base/nsFrameManager.cpp, line 572
###!!! ASSERTION: node in map twice: 'Not Reached', file /scratch/work/builds/trunk.07-06-28.16-46/mozilla/layout/base/nsFrameManager.cpp, line 1677
###!!! ASSERTION: unexpected second call to SetInitialChildList: 'Not Reached', file /scratch/work/builds/trunk.07-06-28.16-46/mozilla/layout/generic/nsContainerFrame.cpp, line 104
###!!! ASSERTION: cannot call on a dirty frame not currently being reflowed: '!NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file /scratch/work/builds/trunk.07-06-28.16-46/mozilla/layout/generic/nsFrame.cpp, line 739

and then 21 more copies of that last assertion.


With Text Box Case:
-------------------
###!!! ASSERTION: Attempt to insert row into wrong map.: 'Error', file /scratch/work/builds/trunk.07-06-28.16-46/mozilla/layout/tables/nsCellMap.cpp, line 596
###!!! ASSERTION: Already have an undisplayed context entry for aContent: '!GetUndisplayedContent(aContent)', file /scratch/work/builds/trunk.07-06-28.16-46/mozilla/layout/base/nsFrameManager.cpp, line 572
###!!! ASSERTION: node in map twice: 'Not Reached', file /scratch/work/builds/trunk.07-06-28.16-46/mozilla/layout/base/nsFrameManager.cpp, line 1677
###!!! ASSERTION: unexpected second call to SetInitialChildList: 'Not Reached', file /scratch/work/builds/trunk.07-06-28.16-46/mozilla/layout/generic/nsContainerFrame.cpp, line 104
###!!! ASSERTION: A box layout method was called but InitBoxMetrics was never called: 'metrics', file /scratch/work/builds/trunk.07-06-28.16-46/mozilla/layout/generic/nsFrame.cpp, line 6380
###!!! ASSERTION: Some frame destructors were not called: 'mFrameCount == 0', file /scratch/work/builds/trunk.07-06-28.16-46/mozilla/layout/base/nsPresShell.cpp, line 672

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> (In both of these cases, I didn't actually
> get the crash)

Oops -- actually, I'm pretty sure I *should* have gotten the crash in the second (with-text-box) case, but I didn't because I added an early return in nsFrame::MarkIntrinsicWidthsDirty to prevent the crash. (for easier debugging)

The crash itself happens within nsFrame::MarkIntrinsicWidthsDirty, and it's triggered because "metrics" is null (as indicated by the assertion at line 6380, in BoxMetrics(), called by MarkIntrinsicWidthsDirty).

Since metrics is null, we get a crash when it's dereferenced at this line (nsFrame.cpp:2916):
    SizeNeedsRecalc(metrics->mPrefSize);

Comment 9

[Daniel Holbert [:dholbert]]

I'm pretty sure that we're vulnerable to the crash partly because of the "unexpected second call to SetInitialChildList" assertion, so I'm focusing on fixing that assertion (at least to start with).

The situation leading to that assertion is this:
 1.  Handle the scripted keypress at nsEventStateManager::PostHandleEvent, at line nsEventStateManager.cpp:2280.  Call ShiftFocus(), because the key press was "tab".

 2. Several levels down, nsEventStateManager::IsFocusable is called, which calls aPresShell->FlushPendingNotifications(Flush_Frames);
 
 3. The flush triggers nsCSSFrameConstructor::ReconstructDocElementHierarchy().  (I think because of the empty <style></style> tag that was encountered but not yet processed?)  This seems to make us rebuild the whole frame tree.

 4.  Stepping down through a few levels of frame-construction, we call nsCSSFrameConstructor::ConstructTableRowGroupFrame, to (re-)create a frame for the span with "display: table-row-group"

 5.  Within that function, call ProcessChildren() at line 3695.

 6.  Down a few levels, we create a frame for the text input box.  This causes this trace of function calls (some levels replaced with ...) 
       nsPlaintextEditor::Init
       ...
       nsTextEditRules::CreateBogusNodeIfNeeded
       nsEditor::InsertNode
       ...
       nsHTMLDivElement::InsertBefore
       ...
       nsContentSink::BeginUpdate
       HTMLContentSink::FlushTags
       ...
       nsBindingManager::ContentAppended
       PresShell::ContentAppended
       nsCSSFrameConstructor::ContentAppended
       nsCSSFrameConstructor::AppendFrames


So, here's what's wrong:  During the ProcessChildren() call in step 5, we __expect__ the frame's child pointer (mFrames) to stay null.  It shouldn't be set set until we call SetInitialChildList, several lines after ProcessChildren.  However, the child pointer (mFrames) **is** modified within ProcessChildren, in that final AppendFrames call in step 6.

So, when we reach SetInitialChildList, we find that our child list is already non-null, and we fail the assertion and bail out of that function without using the child list that was passed in.

This leaves us with an incorrect fixed-list.  (missing most of the frames that it should have)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, hmm.  So really, the key problem is that editor init handles during frame construction. I believe we have some bugs on this already (e.g. it causes frame construction to be reentered in all cases!), and at one point we moved it to be "a little later", as I recall. 

Ideally, we would do editor init once frame construction is done.

As an interim fix here, we could Flush_Frames on the document, not the presshell.  That will flush out the sink before flushing out frames on the presshell.  It'll be slower, since it will flush parent documents, and might destroy the presshell in the process, but as long as we're watching out for that...

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: patch (call Flush_Frames on doc instead of presshell)

This patch implements bz's suggested interim fix from comment 10.  It fixes the crash and all assertions that were originally broken for the testcase, except for this one:

###!!! ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', file /scratch/work/builds/trunk.07-07-20.11-58/mozilla/layout/base/nsPresShell.cpp, line 3101

This patch passes our layout reftests, and I tried navigating the menus and browsing the web a bit and nothing seemed out of the ordinary.

> [it] might destroy the presshell in the process, but as long
> as we're watching out for that...

Should I add code to check for and/or handle the case where we destroy the pres shell?  (not sure how to do so)  Or are we probably safe here, given that we're passing the reftests?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Should I add code to check for and/or handle the case where we destroy the pres
> shell?

If needed, yes.  For example, make sure that there is a stack strong ref to the presshell when calling IsFocusable.  That should do it, since then GetPrimaryFrameFor() will return null, and the ref will keep the object alive.

Or you could just take the strong ref in IsFocusable() itself....  But then you should check the callers to make sure they unwind safely.  I guess you need to do that no matter what...

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to comment #12)
> For example, make sure that there is a stack strong ref to the
> presshell when calling IsFocusable.

Yup, there is.  The only time we call nsEventStateManager.cpp:IsFocusable is at line 4309, in nsEventStateManager::SendFocusBlur.  
(This version of IsFocusable is static and isn't listed in the .h file, so it can't be called anywhere outside of nsEventStateManager.cpp).

And at that point, we have a variable
  nsCOMPtr<nsIPresShell> presShell
so we've got that strong reference.

So, we're ok in that department, I think.

> But then you
> should check the callers to make sure they unwind safely.  I guess you need to
> do that no matter what...

(bz further clarified in IRC that we need to make sure we're "not touching any objects that might have gotten destroyed in the meantime (e.g. frames)" after the call to IsFocusable)

The only issue of this type that I found was with the variable "nsIFrame* mCurrentEventFrame" in PresShell::HandleEventInternal.  We call PreHandleEvent at line 5720, which involves an IsFocusable() call down a few stack frames.  Then, we (unsafely) use the old value of mCurrentEventFrame at line 5735 and 5751.

bz suggested in IRC that this might be solvable by using a nsWeakFrame instead.

Comment 14

[Martijn Wargers (dead)]

The testcase is now worksforme, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007110103 Minefield/3.0a9pre
Can this bug be marked worksforme?

Comment 15

[Daniel Holbert [:dholbert]]

worksforme too, using:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007110715 Minefield/3.0b2pre

Closing.