Closed Bug 141054 Opened 18 years ago Closed 18 years ago

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

Categories

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

x86
Windows 2000
defect

Tracking

()

VERIFIED FIXED
mozilla1.1alpha

People

(Reporter: sspitzer, Assigned: dbaron)

References

()

Details

(Keywords: crash, testcase, topcrash+, Whiteboard: [adt2 RTM])

Crash Data

Attachments

(7 files, 3 obsolete files)

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.
for now, assign to jkeiser.

wild guess on my part, since he changed nsBlockFrame on 4/28
Assignee: ducarroz → jkeiser
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.)
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?
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?
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.
sent mail to dbaron and waterson, so they can reproduce this.

I'll also attach it here.
the HR is the one that mime inserts.
The bug this reminds me of is bug 116022.
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.
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.
This is a crash that occurs during edition therefore this bug should be own by
editor. Reassign
Assignee: jkeiser → kin
Component: Composition → Editor: Core
Keywords: mailtrack
Product: MailNews → Browser
QA Contact: esther → sujay
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).
Do it!
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.
> 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.
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.
Does anyone think that this bug may be related to 144709 with the same symptoms
but a slightly different stack dump?
Bug 145974 is a duplicate of this bug.
*** Bug 146288 has been marked as a duplicate of this bug. ***
nominating since it seems like we're starting to see a few dups.
Keywords: nsbeta1
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.
Depends on: 147888
Attached patch the basic idea of a fix (obsolete) — Splinter Review
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.)
Attached patch more of an idea (obsolete) — Splinter Review
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.)
Attachment #85489 - Attachment is obsolete: true
Attached patch working patch (obsolete) — Splinter Review
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.
Attachment #85533 - Attachment is obsolete: true
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.
Assignee: kin → dbaron
Severity: normal → critical
Priority: -- → P1
Target Milestone: --- → mozilla1.1alpha
The testcase in attachment 85656 [details] also works fine with the patch in attachment 85621 [details] [diff] [review].
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 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
Attachment #85621 - Flags: needs-work+
*** Bug 148274 has been marked as a duplicate of this bug. ***
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?
*** Bug 148263 has been marked as a duplicate of this bug. ***
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.
Summary: reproducable crash in layout [nsLineLayout::IsPercentageUnitSides()] when I select a bunch of html and delete in mail reply → reproducable crash in layout [@ nsLineLayout::IsPercentageUnitSides] when I select a bunch of html and delete in mail reply - M1RC3 [@ nsStyleBorder::IsBorderSideVisible]
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.
But that auto-margin stuff only works thanks to hacks in
nsLineLayout::HorizontalAlignFrames.
Attached patch patch, v. 4Splinter Review
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.
Attachment #85621 - Attachment is obsolete: true
*** Bug 125797 has been marked as a duplicate of this bug. ***
*** Bug 118142 has been marked as a duplicate of this bug. ***
I need to double-check that this isn't causing printfs that I'm seeing in my
debug build...
Attachment #86735 - Flags: superreview+
Comment on attachment 86735 [details] [diff] [review]
patch, v. 4

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

r=kin@netscape.com
Attachment #86735 - Flags: review+
Fix checked in 2002-06-11 20:27 PDT.

Filed bug 151059 on HR centering issues.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [open1.0.1]
mkaply fixed OS/2 bustage - extra semicolons in nsCSSAtomList.h
*** Bug 145974 has been marked as a duplicate of this bug. ***
*** Bug 144709 has been marked as a duplicate of this bug. ***
MArking this as nsbeta1+/[adt2 RTM], since it is a topcrash+ on RC3.
Keywords: nsbeta1adt1.0.1, nsbeta1+
Whiteboard: [open1.0.1] → [open1.0.1] [adt2 RTM]
sujay, can you verify this on the trunk?
I tested the fresh trunk on Win2k. It's ok.

I suggest this patch in branch 1.0.
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.
Keywords: adt1.0.1adt1.0.1+
Keywords: mozilla1.0.1
*** Bug 137630 has been marked as a duplicate of this bug. ***
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
*** Bug 146611 has been marked as a duplicate of this bug. ***
Attachment #86735 - Flags: approval+
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Fix checked in to MOZILLA_1_0_BRANCH, 2002-06-20 16:57 PDT.
Whiteboard: [open1.0.1] [adt2 RTM] → [adt2 RTM]
*** Bug 152973 has been marked as a duplicate of this bug. ***
*** Bug 153853 has been marked as a duplicate of this bug. ***
This fix probably caused regressions bug 154776 and bug 155656.
verified.
Status: RESOLVED → VERIFIED
Keywords: verified1.0.1
*** Bug 151090 has been marked as a duplicate of this bug. ***
Crash Signature: [@ nsLineLayout::IsPercentageUnitSides] [@ nsStyleBorder::IsBorderSideVisible]
You need to log in before you can comment on or make changes to this bug.