171334 - Unusual non-nested containing blocks causes crash in views [@ nsView::GetClippedRect]

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[[e-mail] (gone - use [e-mail] instead)]

Build ID: 2002-09-27-08, Windows 2000.

Summary / Steps to Reproduce:

According to my Talkback stack, trying to view http://www.wheelhouse.com,
crashes in:

1021         zParentChain->ConvertFromParentCoords(&ancestorX, &ancestorY);

Robert, sorry to implicate you, but looks like you were last here ;-(

Probably fallout from bug 168294.

sView::GetClippedRect [c:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 1021]
nsViewManager::UpdateView
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 1610]
nsViewManager::UpdateView
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 1479]
nsViewManager::SetViewContentTransparency
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 2899]
nsHTMLContainerFrame::CreateViewForFrame
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsHTMLContainerFrame.cpp, line
667]
nsCSSFrameConstructor::ConstructFrameByDisplayType
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 6382]
nsCSSFrameConstructor::ConstructFrameInternal
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 7435]
nsCSSFrameConstructor::ConstructFrame
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 7284]
nsCSSFrameConstructor::ProcessChildren
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 12254]
nsCSSFrameConstructor::ConstructTableCellFrame
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 2929]
nsCSSFrameConstructor::TableProcessChild
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 3192]
nsCSSFrameConstructor::TableProcessChildren
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 3087]
nsCSSFrameConstructor::ConstructTableRowFrame
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 2773]
nsCSSFrameConstructor::TableProcessChild
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 3178]
nsCSSFrameConstructor::TableProcessChildren
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 3087]
nsCSSFrameConstructor::ConstructTableRowGroupFrame
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 2664]
nsCSSFrameConstructor::TableProcessChild
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 3172]
nsCSSFrameConstructor::TableProcessChildren
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 3087]
nsCSSFrameConstructor::ConstructTableFrame
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 2545]
nsCSSFrameConstructor::ConstructFrameByDisplayType
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 6603]
nsCSSFrameConstructor::ConstructFrameInternal
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 7435]
nsCSSFrameConstructor::ConstructFrame
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 7284]
nsCSSFrameConstructor::ProcessInlineChildren
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 13743]
nsCSSFrameConstructor::ConstructInline
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 13519]
nsCSSFrameConstructor::ConstructFrameByDisplayType
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 6476]
nsCSSFrameConstructor::ConstructFrameInternal
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 7435]
nsCSSFrameConstructor::ConstructFrame
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 7284]
nsCSSFrameConstructor::ProcessBlockChildren
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 13438]
nsCSSFrameConstructor::ConstructBlock
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 13387]
nsCSSFrameConstructor::ConstructFrameByDisplayType
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 6545]
nsCSSFrameConstructor::ConstructFrameInternal
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 7435]
nsCSSFrameConstructor::ConstructFrame
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 7284]
nsCSSFrameConstructor::ContentAppended
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 8537]
StyleSetImpl::ContentAppended
[c:/builds/seamonkey/mozilla/content/base/src/nsStyleSet.cpp, line 1527]
PresShell::ContentAppended
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 5286]
nsDocument::ContentAppended
[c:/builds/seamonkey/mozilla/content/base/src/nsDocument.cpp, line 2113]
nsHTMLDocument::ContentAppended
[c:/builds/seamonkey/mozilla/content/html/document/src/nsHTMLDocument.cpp, line
1407]
HTMLContentSink::NotifyAppend
[c:/builds/seamonkey/mozilla/content/html/document/src/nsHTMLContentSink.cpp,
line 5271]
SinkContext::FlushTags
[c:/builds/seamonkey/mozilla/content/html/document/src/nsHTMLContentSink.cpp,
line 2228]
HTMLContentSink::OpenHead
[c:/builds/seamonkey/mozilla/content/html/document/src/nsHTMLContentSink.cpp,
line 3246]
CNavDTD::OpenHead [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, line
3115]
CNavDTD::AddHeadLeaf [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp,
line 3840]
CNavDTD::HandleStartToken
[c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, line 1763]
CNavDTD::HandleToken [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp,
line 913]
CNavDTD::BuildModel [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp,
line 530]
nsParser::BuildModel [c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp,
line 1890]
nsParser::ResumeParse [c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp,
line 1754]
nsParser::OnDataAvailable
[c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp, line 2390]
nsDocumentOpenInfo::OnDataAvailable
[c:/builds/seamonkey/mozilla/uriloader/base/nsURILoader.cpp, line 246]
nsHttpChannel::OnDataAvailable
[c:/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp, line 3035]
nsOnDataAvailableEvent::HandleEvent
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsStreamListenerProxy.cpp, line 204]
PL_HandleEvent [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 647]
PL_ProcessPendingEvents [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c,
line 580]
_md_EventReceiverProc [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line
1330]
nsAppShellService::Run
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 472]
main1 [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1538]
main [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1886]
WinMain [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1906]
WinMainCRTStartup()
KERNEL32.DLL + 0x1ca90 (0x77e9ca90) 

Additional Info:

x86 Registers:
EAX: 00000000 EBX: 02da7158 ECX: 00000000 EDX: 029419c8
ESI: 00000000 EDI: 00000000 ESP: 0012e548 EBP: 0012e56c
EIP: 61381ede CF pf AF zf SF of IF df nt RF vm   IOPL: 0
CS: 001b DS: 0023 SS: 0023 ES: 0023 FS: 0038 GS: 0000
Code Around the PC:
61381ede 2b792c           sub     edi,[ecx+0x2c]
61381ee1 8b7130           mov     esi,[ecx+0x30]
61381ee4 8b4908           mov     ecx,[ecx+0x8]
61381ee7 2975fc           sub     [ebp-0x4],esi
61381eea 3bcb             cmp     ecx,ebx
61381eec 75f0             jnz     61381ede
61381eee 897df8           mov     [ebp-0x8],edi
61381ef1 8bda             mov     ebx,edx
61381ef3 85db             test    ebx,ebx
61381ef5 746c             jz      61381f63
61381ef7 8b5358           mov     edx,[ebx+0x58]
61381efa 33f6             xor     esi,esi
61381efc 8bca             mov     ecx,edx

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, this is mine.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: simplified testcase

This testcase demonstrates the bug. You may not be able to get it to crash, but
it constructs the same invalid view hierarchy as at wheelhouse.com.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What appears to be happening is that the block-in-inline code is moving the
outer TABLE frame to be a sibling of the relatively-positioned SPAN. However,
the absolutely-positioned SPAN inside the TABLE still gets the
relatively-positioned SPAN as its containing block. Thus the
absolutely-positioned SPAN's containing block is NOT one of its ancestors, which
screws up views (and probably other things).

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK ... so here's more analysis:

The table is align="left", so it gets style "float:left". This triggers the code
      if (isFloating) {
        geometricParent = aState.mFloatedItems.containingBlock;
      }
in nsCSSFrameConstructor::ConstructFrameByDisplayType. The problem is that now
we have set the table's geometric parent to the containing block for floaters
(which is the BODY, I think), but the frame construction state has a different
idea of what the containing block for absolutes should be --- in this case, a
child block of the floater containing block.

So it seems we should reset aState.mAbsoluteItems.containingBlock inside the
above "if (isFloating)".

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: insanity

This insane testcase shows how ugly things can get. There is no proper nesting
of containing blocks in CSS2.1.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Here's what has to be done:

-- We have to create a view for each float, so we can record in the view what
the "zParent" (aka content parent) view is for the float. This should also fix a
bug we undoubtedly have where floats inside relatively-positioned elements
aren't put into the right z-index stacking context.
-- We have to modify nsCSSFrameConstructor so that the contentParent frame is
passed everywhere, including into TABLE construction, where it can be handed to
nsHTMLContainerFrame::CreateViewForFrame.
-- We have to modify the view system to not rely on the assumption that a view's
containing parent view is an containing ancestor of (or equal to) the view's
content parent view. (It is, however, a content ancestor of the view's content
parent view.)

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I will do the last step first (remove the view system's invalid assumption about
containing block containment), which will fix the crasher and make these
examples work. Then I'll do the other two steps in a separate patch.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Proposed patch

This needs a bit more testing. In particular it's hard to see if the new logic
in nsView.cpp is working, because there are other bugs which make absolutely
positioned content inside relatives not display.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: comprehensive fix

This patch fixes the bug completely. The 'insanity' testcase now works 100%
correctly! (OK, some assertions in layout fire, but the result is correct.)

The patch does a few things:
-- Get rid of nsView "compositor flags" and change the only use of them to
something much simpler
-- The main point: remove the assumption in views that the geometric parent of
a view is a geometric ancestor of the view's content parent. This accounts for
the changes in CreateDisplayList and nsView::GetClippedRect. The main idea is
to not reparent during CreateDisplayList but to do all reparenting in one pass
over the entire tree at the end. We also modify an optimization that's unsafe
in the presence of arbitrary reparenting. This is what actually fixes the
crasher.
-- To actually test this properly and make sure that reparenting was working in
evil cases like the 'insanity' test, I had to fix a longstanding bug involving
relatively positioned elements with absolutely positioned children. It turned
out to be simply a matter of turning back on some code that was turned off two
and a half years ago... This fix should fix a lot of bugs reported against
relative positioning.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My last comment is not quite true. As you can see, to fix the problems with
relative positioning, I had to change a call to SyncFrameViewAfterReflow to
PositionFrameView. The SyncFrameViewAfterReflow call was passing in "null" for
the overflowarea, basically chopping off any overflow.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note that we don't need to call SyncFrameViewAfterReflow because it only sizes
and positions the view, and the size has already been set by a call to
ResizeView when the inline frame was reflowed.

Comment 12

[karnaze (gone)]

r=karnaze for the changes to nsBlockFrame and nsInlineFrame. I think the patch
I'm working on for splitting abs pos frames does the same thing to
nsInlineFrame, and it sounds like this patch will help that effort in other
ways. I'm not as familar with the change to nsBlockFrame, but comment #11 sounds
like a reasonable explanation.

Comment 13

[Kevin McCluskey (gone)]

Comment on attachment 101658 [details] [diff] [review]
comprehensive fix

r=[e-mail] for view module changes

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

*** Bug 173062 has been marked as a duplicate of this bug. ***

Comment 15

[greer]

topcrash on the the Trunk && testcase -> topcrash+. (Just a formality for the
automation.)

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

kin found a problem in the patch. I'll post another patch soon.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

To simplify things I have decided to split the patch into two orthogonal pieces:
a views patch which fixes the crasher here, and a layout patch which fixes the
views-on-inlines issues. I will attach the layout patch to 131475. The patches
are independent.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Views patch

This is just the changes to the view system. Alone, they fix the crasher.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 102411 [details] [diff] [review]
Views patch

carrying forward r=kmcclusk

Comment 20

[kinmoz]

Comment on attachment 102411 [details] [diff] [review]
Views patch

sr=[e-mail]

==== Make sure the lines in your new comments are less than 80 chars.

==== Can you put some returns between the NS_VIEW_FLAG_* defines in nsView.h.
so that they are a bit easier to read?

==== I must admit that this was a bit confusing to me (how can something be
totally opaque and transparent) until you explained the use of the
NS_VIEW_FLAG_TRANSPARENT flag to me. Perhaps the comment for this flag in
nsView.h should mention how it is used?


-  mVFlags = 0;
+  mVFlags = NS_VIEW_FLAG_TRANSPARENT;
   mOpacity = 1.0f;

Comment 21

[Olivier Cahagne]

*** Bug 173787 has been marked as a duplicate of this bug. ***

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 102411 [details] [diff] [review]
Views patch

a=asa for checkin to 1.2beta (on behalf of drivers)

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

fix checked in.

Comment 24

[[e-mail] (gone - use [e-mail] instead)]

Changing testcase URL from http://www.wheelhouse.com (since it's down ATM) with
http://geocities.com/moz_blubber.

Comment 25

[Greg K.]

Reproduced using FizzillaCFM/2002100308. Setting All/All.

Comment 26

[Matt Seitz]

WFM on Windows NT with build 2002101108

Comment 27

[[e-mail] (gone - use [e-mail] instead)]

Verified FIXED with trunk 2002-10-13-08 builds on RedHat 8.0, Windows 2000 and
Mac OS X 10.1.5