141054 - reproducable crash in layout [@ nsLineLayout::IsPercentageUnitSides] when I select a bunch of html and delete in mail reply - M1RC3 [@ nsStyleBorder::IsBorderSideVisible]

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[(not reading, please use [e-mail] instead)]

reproducable crash in layout [nsLineLayout::IsPercentageUnitSides()] when I
select a bunch of html and delete in mail reply

I'm replying to a certain forwarded message, scrolling to the bottom, select up,
and delete.

the message is sent page from imdb.com, so there is heavy html.

the crash is because aSides is null.

http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsLineLayout.cpp#1770

here's the stack:

nsLineLayout::IsPercentageUnitSides   [nsLineLayout.cpp, line 1776]        
IsPercentageAwareChild   [nsBlockFrame.cpp, line 1070]        
nsBlockFrame::ReflowInlineFrame   [nsBlockFrame.cpp, line 3725]        
nsBlockFrame::DoReflowInlineFrames   [nsBlockFrame.cpp, line 3456]        
nsBlockFrame::DoReflowInlineFramesAuto   [nsBlockFrame.cpp, line 3381]        
nsBlockFrame::ReflowInlineFrames   [nsBlockFrame.cpp, line 3326]        
nsBlockFrame::ReflowLine   [nsBlockFrame.cpp, line 2484]        
nsBlockFrame::ReflowDirtyLines   [nsBlockFrame.cpp, line 2128]        
nsBlockFrame::Reflow   [nsBlockFrame.cpp, line 864]        
nsBlockReflowContext::DoReflowBlock   [nsBlockReflowContext.cpp, line 581]     
   nsBlockReflowContext::ReflowBlock   [nsBlockReflowContext.cpp, line 359]    
    nsBlockFrame::ReflowBlockFrame   [nsBlockFrame.cpp, line 3082]        
nsBlockFrame::ReflowLine   [nsBlockFrame.cpp, line 2350]        
nsBlockFrame::ReflowDirtyLines   [nsBlockFrame.cpp, line 2128]        
nsBlockFrame::Reflow   [nsBlockFrame.cpp, line 864]        
nsBlockReflowContext::DoReflowBlock   [nsBlockReflowContext.cpp, line 581]     
   nsBlockReflowContext::ReflowBlock   [nsBlockReflowContext.cpp, line 359]    
    nsBlockFrame::ReflowBlockFrame   [nsBlockFrame.cpp, line 3082]        
nsBlockFrame::ReflowLine   [nsBlockFrame.cpp, line 2350]        
nsBlockFrame::ReflowDirtyLines   [nsBlockFrame.cpp, line 2128]        
nsBlockFrame::Reflow   [nsBlockFrame.cpp, line 864]        
nsContainerFrame::ReflowChild   [nsContainerFrame.cpp, line 807]        
CanvasFrame::Reflow   [nsHTMLFrame.cpp, line 565]        
nsBoxToBlockAdaptor::Reflow   [nsBoxToBlockAdaptor.cpp, line 837]        
nsBoxToBlockAdaptor::DoLayout   [nsBoxToBlockAdaptor.cpp, line 619]        
nsBox::Layout   [nsBox.cpp, line 1052]         nsScrollBoxFrame::DoLayout  
[nsScrollBoxFrame.cpp, line 395]         nsBox::Layout   [nsBox.cpp, line 1052]
        nsContainerBox::LayoutChildAt   [nsContainerBox.cpp, line 650]        
nsGfxScrollFrameInner::LayoutBox   [nsGfxScrollFrame.cpp, line 1063]        
nsGfxScrollFrameInner::Layout   [nsGfxScrollFrame.cpp, line 1222]        
nsGfxScrollFrame::DoLayout   [nsGfxScrollFrame.cpp, line 1071]        
nsBox::Layout   [nsBox.cpp, line 1052]         nsBoxFrame::Reflow  
[nsBoxFrame.cpp, line 1001]         nsGfxScrollFrame::Reflow  
[nsGfxScrollFrame.cpp, line 780]         nsContainerFrame::ReflowChild  
[nsContainerFrame.cpp, line 807]         ViewportFrame::Reflow  
[nsViewportFrame.cpp, line 588]         nsHTMLReflowCommand::Dispatch  
[nsHTMLReflowCommand.cpp, line 224]         PresShell::ProcessReflowCommand  
[nsPresShell.cpp, line 6194]         PresShell::ProcessReflowCommands  
[nsPresShell.cpp, line 6249]         PresShell::FlushPendingNotifications  
[nsPresShell.cpp, line 5049]         PresShell::EndReflowBatching  
[nsPresShell.cpp, line 5080]         nsEditor::EndUpdateViewBatch  
[nsEditor.cpp, line 4270]         nsEditor::EndPlaceHolderTransaction  
[nsEditor.cpp, line 715]         nsPlaintextEditor::DeleteSelection  
[nsPlaintextEditor.cpp, line 952]         nsTextEditorKeyListener::KeyPress  
[nsEditorEventListeners.cpp, line 243]        
nsEventListenerManager::HandleEvent   [nsEventListenerManager.cpp, line 1656]  
      nsDocument::HandleDOMEvent   [nsDocument.cpp, line 3412]        
nsGenericElement::HandleDOMEvent   [nsGenericElement.cpp, line 1697]        
PresShell::HandleEventInternal   [nsPresShell.cpp, line 5994]        
PresShell::HandleEvent   [nsPresShell.cpp, line 5917]        
nsViewManager::HandleEvent   [nsViewManager.cpp, line 2030]        
nsView::HandleEvent   [nsView.cpp, line 306]        
nsViewManager::DispatchEvent   [nsViewManager.cpp, line 1887]        
HandleEvent   [nsView.cpp, line 83]         nsWindow::DispatchEvent  
[nsWindow.cpp, line 869]         nsWindow::DispatchWindowEvent   [nsWindow.cpp,
line 886]         nsWindow::DispatchKeyEvent   [nsWindow.cpp, line 2660]       
 nsWindow::OnChar   [nsWindow.cpp, line 2810]         nsWindow::ProcessMessage 
 [nsWindow.cpp, line 3459]         nsWindow::WindowProc   [nsWindow.cpp, line
1131]         USER32.DLL + 0x3eb0 (0x77e13eb0)             USER32.DLL + 0x401a
(0x77e1401a)             USER32.DLL + 0x92da (0x77e192da)    

here's a talkback:

http://climate.netscape.com/reports/incidenttemplate.cfm?bbid=5752115

this was on today's official trunk, mozilla build, win2k.

Comment 1

[(not reading, please use [e-mail] instead)]

for now, assign to jkeiser.

wild guess on my part, since he changed nsBlockFrame on 4/28

Comment 2

[Chris Waterson]

Probably not jkeiser's fault. Why would GetStyleData be returning a null style
struct? sspitzer, which call to aFrame->GetStyleData is filling in the null
pointer? (Can't tell from the line numbers above -- looks to be pointing
directly at the function itself.)

Comment 3

[John Keiser (jkeiser)]

Yes, the reflow stack occurs before my changes (which are near the end of the
reflow).  Not to mention, my stuff didn't change any logic.

Did this not happen on the 4/27 and 4/28 builds?

Comment 4

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

Is the inline frame in question an HR?  I feel like I've seen this bug before.

Is there a way someone else could reproduce to debug it?  Do I just need a mail
folder with a message with a sent page from www.imdb.com?

Comment 5

[(not reading, please use [e-mail] instead)]

my apologizes to jkeiser, this is not that new.

I see it in a 20020425 mozilla 0.9.9+ build.

let me try to answer waterson's question, and attach the message.

or, I'll send a mail to dbaron and waterson, so they can try it.

to answer dbaron's question, yes, a HR frame might be involved.

Comment 6

[(not reading, please use [e-mail] instead)]

sent mail to dbaron and waterson, so they can reproduce this.

I'll also attach it here.

Comment 7

[(not reading, please use [e-mail] instead)]

Attachment: mbox file with the evil message, save to disk under "Local Folders"

Comment 8

[(not reading, please use [e-mail] instead)]

the HR is the one that mime inserts.

Comment 9

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

The bug this reminds me of is bug 116022.

Comment 10

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

So the crash is happening because we're going through a deleted rule node.  The
style context parentage printfs that spew out a little before the crash are
relevant.

Comment 11

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

I think half the problem is bug 141261 (just like the patch at bug 115919
comment 31) and half the problem is that the patch at bug 115919 comment 25
didn't fix the whole RemoveGeneratedContentFrameSiblings problem (as suggested
by bug 126072, although I think there was also a bug more directly related).

Some of the style context warnings may be bogus -- see bug 141259.

Comment 12

[Jean-Francois Ducarroz]

This is a crash that occurs during edition therefore this bug should be own by
editor. Reassign

Comment 13

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

Based on debugging, the problem with RemoveGeneratedContentFrameSiblings seems
to be in removing the :after frame.  I see the assertion in the following patch,
so the problem seems to be with RemoveGeneratedContentFrameSiblings not handling
some case or other:

Index: nsCSSFrameConstructor.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,v
retrieving revision 1.737
diff -u -d -r1.737 nsCSSFrameConstructor.cpp
--- nsCSSFrameConstructor.cpp	1 May 2002 00:51:36 -0000	1.737
+++ nsCSSFrameConstructor.cpp	3 May 2002 21:05:53 -0000
@@ -9535,6 +9535,9 @@
     }
   }
 
+#ifdef DEBUG_dbaron
+  PRBool hadBefore = PR_FALSE;
+#endif
   if (beforeFrame &&
       IsGeneratedContentFor(content, beforeFrame, nsCSSAtoms::beforePseudo)) {
     // Do we need to call something like |DeletingFrameSubtree| here?
@@ -9543,6 +9546,9 @@
     aFrameManager->RemoveFrame(aPresContext, *aPresShell,
                                aInsertionPoint, nsnull,
                                beforeFrame);
+#ifdef DEBUG_dbaron
+    hadBefore = PR_TRUE;
+#endif
   }
 
   // Remove any :after generated content frame that follows aFrame.
@@ -9574,6 +9580,15 @@
                                aInsertionPoint, nsnull,
                                afterFrame);
   }
+#ifdef DEBUG_dbaron
+  else if (hadBefore) {
+    nsCOMPtr<nsIContent> content;
+    aFrame->GetContent(getter_AddRefs(content));
+    nsCOMPtr<nsIAtom> tag;
+    content->GetTag(*getter_AddRefs(tag));
+    NS_ASSERTION(tag != nsHTMLAtoms::hr, "couldn't find hr:after");
+  }
+#endif
 }
 
 NS_IMETHODIMP


I'm "*this*" close to ripping out all of this leaf node generated content stuff
(since it's probably wrong according to CSS) and hacking in the quirks-mode HR
frame construction manually (as children of a common inline primary frame).

Comment 14

[Chris Waterson]

Do it!

Comment 15

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

I took another look at this bug, and I'm more confused than I was last time.  I
put in debugging code to do a *full* frame tree dump at:
 * the beginning of every call to ViewportFrame::Reflow, and
 * the beginning of every call to RemoveGeneratedContentFrameSiblings
 * every time RemoveGeneratedContentFrameSiblings fails my assertions in the
patch above

The frame on which we crash is an inline frame with a style context for
hr:after, and the next line has another such frame, but there are no frames for
hr or hr:before in the area.

However, the first time either of these frames' addresses shows up in my
debugging output is when we're about to reflow the viewport frame, but in that
frame dump, they're already orphaned (no hr or hr:before frames).  This suggests
to me that something else is messing with these frames other than
RemoveGeneratedContentFrameSiblings.

Comment 16

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

> The frame on which we crash is an inline frame with a style context for
> hr:after, and the next line has another such frame

Oh, wait, I started tracing the content nodes, and I'm seeing something
interesting -- in other words, I found the other 2 frames at a totally different
place.  It looks like the relevant separation is inline-block splitting.  I'll
attach a frame dump, with "HR #1" and "HR #2" notes in the left margin.

Comment 17

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

Attachment: frame dump, with "HR #1" and "HR #2" notes in left margin

Comment 18

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

So I think the best solution here (considering that the working group considers
that :before and :after should work on replaced elements) is probably to create
an additional frame around the :before and :after content, that is block display
if the frame it contains is block display, and otherwise inline, and otherwise
"inherits" no styles from its child, the real frame.

This raises two tricky issues:
 1. how to do the style context parenting
 2. which frame should be the primary frame.

I think the answers to these are:
 1. The frames for the replaced element and its :before and :after) should have
the same parent style contexts as now.  (Now that I think about this, right now
we're probably re-resolving the :before and :after incorrectly, which is a bug.)
 The new frame's style context should be resolved to one of 2 pseudo elements,
with a parent the same as the parent of the current primary frame.  (Or should
the parent *be* the current primary frame?  That seems to make more sense, but
it would be "fun" like the table stuff is now.)  This would mean we'd need to
change GetParentStyleContextFrame to skip frames with one of these 2 new pseudos
when finding the parent.  However, it should really already be skipping frames
with many other pseudos (as should the frame construction code, which also
doesn't), such as the ones for anonymous table objects.  Perhaps
GetParentStyleContextFrame should be comparing content nodes of frames, and
pseudo-elements on their style contexts?  I'm not sure.  I think the whole model
is broken.  (Perhaps we should have a style context tree with pointers into the
frame tree rather than vice-versa.)
 2. The inner frame (the one that is now) should be the primary frame, and
ContentRemoved should just do a GetParent and a GetContent to see if it needs to
walk up one.

Comment 19

[Cameron Kellough]

Does anyone think that this bug may be related to 144709 with the same symptoms
but a slightly different stack dump?

Comment 20

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

Bug 145974 is a duplicate of this bug.

Comment 21

[Jean-Francois Ducarroz]

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

Comment 22

[scottputterman]

nominating since it seems like we're starting to see a few dups.

Comment 23

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

Note:  There is a difference in branch/trunk behavior in the code in this area,
and if we fix this on the branch we may want to land the fix for bug 141289 on
the branch as well.

Comment 24

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

Attachment: the basic idea of a fix

This is the basic idea of what needs to be done -- there's a bit of code that
needs to be filled in.	I've realized that the bugs caused by the weird style
context parenting in this approach won't really be very serious, so we can just
fix this first, and then fix bug 147888.

(The main reason I'm attaching this is really that I'm storing one computer and
replacing it with another, really...  It doesn't even compile yet.)

Comment 25

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

Attachment: more of an idea

This should be quite a bit closer to what we need.  It's an attempt at wrapper
frame construction with the incorrect style context parenting.	It doesn't work
yet, though.  I suspect something's wrong with my frame tree mangling.

This suppresses construction of generated content for absolute/fixed positioned
or floated leaf frames.  That's not too bad, considering I wanted to rip it out
entirely, and it makes this no harder than just doing it for HR.  Doing those
correctly would require more style context manipulation than I really want to
think about doing.

Any thoughts on the basic idea here?  (Yes, I know the optionally null in-out
parameter is ugly.  I couldn't think of a better way without rearranging lots
of code.)

Comment 26

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

Attachment: working patch

This one actually works.  HRs still work, and I can edit the email message
without crashing.  I need to do more testing of HRs, and I need to double-check
that I didn't do anything too evil to the style contexts.

Comment 27

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

I can take the bug, too, while I'm at it...

Setting 1.1alpha since I'm unlikely to look at my 1.0.1 list (it's empty, I
think), and it's really both, since they're parallel.

Comment 28

[kinmoz]

Attachment: Simple JS Test Case for Viewer/Browser

Comment 29

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

The testcase in attachment 85656 [details] also works fine with the patch in attachment 85621 [details] [diff] [review].

Comment 30

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

Attachment: testcase showing images with generated content

This is a testcase showing that generated content around 'display: inline' and
'display: block' images still works.  The behavior on the block image may be
different -- I have to check the old behavior, but I think it's reasonable and
I doubt anybody really cares.  (The patch makes the behavior so that there are
line breaks before the :before generated content, between the :before generated
content and the replaced content (the image), between the replaced content and
the :after generated content, and after the :after generated content.)

Comment 31

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

Comment on attachment 85621 [details] [diff] [review]
working patch

So this breaks centered hr elements (<center><hr width="foo"></center>) like
the ones in http://mozilla.org/party/2002/faq.html

Comment 32

[R.K.Aa.]

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

Comment 33

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

Attachment: simple testcase for what this breaks

This testcase is simply '<hr width="50">'.  It's centered in trunk Mozilla, but
this patch makes it left-aligned.  I'm still trying to figure out what in the
current code makes it centered.  Anybody have any ideas?

Comment 34

[Jay Patel [:jay]]

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

Comment 35

[Jay Patel [:jay]]

Adding M1RC3 [@ nsStyleBorder::IsBorderSideVisible] to summary from duped Bug
148263.  A very similar crash to the on reported in this bug is a topcrasher for
Mozilla 1.0 RC3.

Adding crash, topcrash and testcase keywords.

Comment 36

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

So I guess I figured out the HR centering problem.  We currently have a (4.xP)
bug in quirks mode (I didn't test standard mode) that while

<p><hr width="50"></p>

is centered correctly,
 
<p><b><hr width="50"></b></p>

is not centered correctly.  By constructing an inline frame around the hr, we
run into this bug.

This is because the existing centering is done by a rule in html.css:

margin: 0 auto 0 auto;

So I need to figure out a new way to center HR elements in quirks mode.

Comment 37

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

But that auto-margin stuff only works thanks to hacks in
nsLineLayout::HorizontalAlignFrames.

Comment 38

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

Attachment: a few more hr testcases

Comment 39

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

Attachment: patch, v. 4

This fixes the centering regression by changing a frame-type check to a content
type check.  As the previous attachment shows, this check isn't sufficient. 
However, it restores the current bugginess to the same level.

Comment 40

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

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

Comment 41

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

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

Comment 42

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

I need to double-check that this isn't causing printfs that I'm seeing in my
debug build...

Comment 43

[Chris Waterson]

Comment on attachment 86735 [details] [diff] [review]
patch, v. 4

sr=waterson

Comment 44

[kinmoz]

Comment on attachment 86735 [details] [diff] [review]
patch, v. 4

r=[e-mail]

Comment 45

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

Fix checked in 2002-06-11 20:27 PDT.

Filed bug 151059 on HR centering issues.

Comment 46

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

mkaply fixed OS/2 bustage - extra semicolons in nsCSSAtomList.h

Comment 47

[Jean-Francois Ducarroz]

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

Comment 48

[Henry Jia]

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

Comment 49

[Jaime Rodriguez, Jr.]

MArking this as nsbeta1+/[adt2 RTM], since it is a topcrash+ on RC3.

Comment 50

[scottputterman]

sujay, can you verify this on the trunk?

Comment 51

[Henry Jia]

I tested the fresh trunk on Win2k. It's ok.

I suggest this patch in branch 1.0.

Comment 52

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch. pls check this in
asap, then add the "fixed1.0.1" keyword.

Comment 53

[kinmoz]

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

Comment 54

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 55

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

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

Comment 56

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 57

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

Fix checked in to MOZILLA_1_0_BRANCH, 2002-06-20 16:57 PDT.

Comment 58

[kinmoz]

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

Comment 59

[Jean-Francois Ducarroz]

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

Comment 60

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

This fix probably caused regressions bug 154776 and bug 155656.

Comment 61

[sujay]

verified.

Comment 62

[Tristan Seligmann]

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