55250 - crash dynamically changing "align" attribute of img, object, or table

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Nasiruddin M.S]

Build id: 2000092723
Platform: Solaris2.7(sparc)

(1) open the composer.
(2) insert a 'jpg' image in the page using insert->image option.
(3)  save the file and close the composer.
(4) Open the composer again.And open the file saved previously.
(5) Double click on the image,which will bring up a image properties dialog box.
(6) In the image properties dialog box select the option 'wrap to the right' or
'wrap to the left' for 'align text to image' option.
(7) click on 'ok' button.
(8) Now type some alphabet (say 'p')

The browser crashes suddenly.

Comment 1

[Simon Fraser [no longer active]]

Yup, nasty crash. Anthony, please look at this for RTM.

Comment 2

[Simon Fraser [no longer active]]

Nominating as RTM+

Comment 3

[Simon Fraser [no longer active]]

All platforms.

Comment 4

[anthonyd]

stack trace from windows:
PlaceFrameView(nsIPresContext * 0x040ccce0, nsIFrame * 0x00000000) line 3433 + 
11 bytes
nsBlockFrame::PostPlaceLine(nsBlockReflowState & {...}, nsLineBox * 0x00e61f60, 
const nsSize & {...}) line 4944 + 16 bytes
nsBlockFrame::PlaceLine(nsBlockReflowState & {...}, nsLineLayout & {...}, 
nsLineBox * 0x00e61f60, int * 0x0012d130, int 0) line 4807
nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState & {...}, nsLineLayout & 
{...}, nsLineBox * 0x00e61f60, int * 0x0012d130, unsigned char * 0x0012cf78, int 
0, int 1) line 4313 + 28 bytes
nsBlockFrame::DoReflowInlineFramesAuto(nsBlockReflowState & {...}, nsLineBox * 
0x00e61f60, int * 0x0012d130, unsigned char * 0x0012cf78, int 0, int 1) line 
4175 + 42 bytes
nsBlockFrame::ReflowInlineFrames(nsBlockReflowState & {...}, nsLineBox * 
0x00e61f60, int * 0x0012d130, int 1, int 0) line 4120 + 32 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x00e61f60, int 
* 0x0012d130, int 1) line 3256 + 29 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2945 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x00e80f0c, nsIPresContext * 
0x040ccce0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 1746 + 15 bytes
nsBlockReflowContext::DoReflowBlock(nsHTMLReflowState & {...}, nsReflowReason 
eReflowReason_Dirty, nsIFrame * 0x00e80f0c, const nsRect & {...}, int 1, int 0, 
int 1, nsMargin & {...}, unsigned int & 0) line 561 + 36 bytes
nsBlockReflowContext::ReflowBlock(nsIFrame * 0x00e80f0c, const nsRect & {...}, 
int 1, int 0, int 1, nsMargin & {...}, unsigned int & 0) line 331 + 50 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineBox * 
0x00e61fb0, int * 0x0012dc68) line 3873 + 56 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x00e61fb0, int 
* 0x0012dc68, int 1) line 3138 + 23 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2945 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x00e80e84, nsIPresContext * 
0x040ccce0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 1746 + 15 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x00e80e84, nsIPresContext * 
0x040ccce0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, 
int 0, unsigned int 0, unsigned int & 0) line 693 + 31 bytes
CanvasFrame::Reflow(CanvasFrame * const 0x00e8000c, nsIPresContext * 0x040ccce0, 
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) 
line 306
nsBoxToBlockAdaptor::Reflow(nsBoxLayoutState & {...}, nsIPresContext * 
0x040ccce0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0, int 0, int 0, int 9675, int 8145, int 1) line 868
nsBoxToBlockAdaptor::DoLayout(nsBoxToBlockAdaptor * const 0x00e80e18, 
nsBoxLayoutState & {...}) line 525 + 52 bytes
nsBox::Layout(nsBox * const 0x00e80e18, nsBoxLayoutState & {...}) line 1002
nsScrollBoxFrame::DoLayout(nsScrollBoxFrame * const 0x00e80124, nsBoxLayoutState 
& {...}) line 378
nsBox::Layout(nsBox * const 0x00e80124, nsBoxLayoutState & {...}) line 1002
nsContainerBox::LayoutChildAt(nsBoxLayoutState & {...}, nsIBox * 0x00e80124, 
const nsRect & {...}) line 593 + 16 bytes
nsGfxScrollFrameInner::LayoutBox(nsBoxLayoutState & {...}, nsIBox * 0x00e80124, 
const nsRect & {...}) line 1063 + 17 bytes
nsGfxScrollFrameInner::Layout(nsBoxLayoutState & {...}) line 1146
nsGfxScrollFrame::DoLayout(nsGfxScrollFrame * const 0x00e80080, nsBoxLayoutState 
& {...}) line 1071 + 15 bytes
nsBox::Layout(nsBox * const 0x00e80080, nsBoxLayoutState & {...}) line 1002
nsBoxFrame::Reflow(nsBoxFrame * const 0x00e80048, nsIPresContext * 0x040ccce0, 
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) 
line 775
nsGfxScrollFrame::Reflow(nsGfxScrollFrame * const 0x00e80048, nsIPresContext * 
0x040ccce0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 775 + 25 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x00e80048, nsIPresContext * 
0x040ccce0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, 
int 0, unsigned int 0, unsigned int & 0) line 693 + 31 bytes
ViewportFrame::Reflow(ViewportFrame * const 0x00e7ffd0, nsIPresContext * 
0x040ccce0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 546
nsHTMLReflowCommand::Dispatch(nsHTMLReflowCommand * const 0x03f64f40, 
nsIPresContext * 0x040ccce0, nsHTMLReflowMetrics & {...}, const nsSize & {...}, 
nsIRenderingContext & {...}) line 146
PresShell::ProcessReflowCommands(int 0) line 4475
PresShell::FlushPendingNotifications(PresShell * const 0x0409a390) line 3563
PresShell::EndReflowBatching(PresShell * const 0x0409a390, int 1) line 3586 + 15 
bytes
nsEditor::EndUpdateViewBatch() line 5375
nsEditor::EndPlaceHolderTransaction(nsEditor * const 0x03f779b0) line 1368
nsAutoPlaceHolderBatch::~nsAutoPlaceHolderBatch() line 48 + 44 bytes
nsHTMLEditor::TypedText(nsHTMLEditor * const 0x03f77a48, const nsString & {...}, 
int 0) line 797 + 27 bytes
nsHTMLEditorLog::TypedText(nsHTMLEditorLog * const 0x03f77a48, const nsString & 
{...}, int 0) line 180 + 17 bytes
nsHTMLEditor::EditorKeyPress(nsHTMLEditor * const 0x03f77a48, nsIDOMKeyEvent * 
0x03f4d090) line 777 + 21 bytes
nsTextEditorKeyListener::KeyPress(nsIDOMEvent * 0x03f4d094) line 264 + 41 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x040ccce0, nsEvent * 
0x0012f834, nsIDOMEvent * * 0x0012f550, nsIDOMEventTarget * 0x040c82fc, unsigned 
int 2, nsEventStatus * 0x0012f7a0) line 1118 + 23 bytes
nsDocument::HandleDOMEvent(nsDocument * const 0x040c82d0, nsIPresContext * 
0x040ccce0, nsEvent * 0x0012f834, nsIDOMEvent * * 0x0012f550, unsigned int 2, 
nsEventStatus * 0x0012f7a0) line 3046
nsGenericElement::HandleDOMEvent(nsIPresContext * 0x040ccce0, nsEvent * 
0x0012f834, nsIDOMEvent * * 0x0012f550, unsigned int 1, nsEventStatus * 
0x0012f7a0) line 1433 + 45 bytes
nsHTMLHtmlElement::HandleDOMEvent(nsHTMLHtmlElement * const 0x040caee8, 
nsIPresContext * 0x040ccce0, nsEvent * 0x0012f834, nsIDOMEvent * * 0x00000000, 
unsigned int 1, nsEventStatus * 0x0012f7a0) line 186
PresShell::HandleEventInternal(nsEvent * 0x0012f834, nsIView * 0x03f6aa50, 
unsigned int 1, nsEventStatus * 0x0012f7a0) line 4255 + 47 bytes
PresShell::HandleEvent(PresShell * const 0x0409a394, nsIView * 0x03f6aa50, 
nsGUIEvent * 0x0012f834, nsEventStatus * 0x0012f7a0, int 0, int & 1) line 4190 + 
25 bytes
nsView::HandleEvent(nsView * const 0x03f6aa50, nsGUIEvent * 0x0012f834, unsigned 
int 8, nsEventStatus * 0x0012f7a0, int 0, int & 1) line 379
nsView::HandleEvent(nsView * const 0x03f6f1b0, nsGUIEvent * 0x0012f834, unsigned 
int 8, nsEventStatus * 0x0012f7a0, int 0, int & 1) line 352
nsView::HandleEvent(nsView * const 0x04096a60, nsGUIEvent * 0x0012f834, unsigned 
int 28, nsEventStatus * 0x0012f7a0, int 1, int & 1) line 352
nsViewManager2::DispatchEvent(nsViewManager2 * const 0x04091990, nsGUIEvent * 
0x0012f834, nsEventStatus * 0x0012f7a0) line 1439
HandleEvent(nsGUIEvent * 0x0012f834) line 68
nsWindow::DispatchEvent(nsWindow * const 0x04096924, nsGUIEvent * 0x0012f834, 
nsEventStatus & nsEventStatus_eIgnore) line 681 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f834) line 702
nsWindow::DispatchKeyEvent(unsigned int 131, unsigned short 112, unsigned int 0) 
line 2284 + 15 bytes
nsWindow::OnChar(unsigned int 112, unsigned int 0, unsigned char 0) line 2408
nsWindow::ProcessMessage(unsigned int 258, unsigned int 112, long 1638401, long 
* 0x0012fbbc) line 2841 + 33 bytes
nsWindow::WindowProc(HWND__ * 0x0010018e, unsigned int 258, unsigned int 112, 
long 1638401) line 950 + 27 bytes
USER32! 77e148dc()
USER32! 77e14aa7()
USER32! 77e266fd()
nsAppShellService::Run(nsAppShellService * const 0x00578e20) line 408
main1(int 1, char * * 0x004a3260, nsISupports * 0x00000000) line 1004 + 32 bytes
main(int 1, char * * 0x004a3260) line 1185 + 37 bytes
mainCRTStartup() line 338 + 17 bytes

Comment 5

[Simon Fraser [no longer active]]

Looks like a crash in layout. cc layout folks

Comment 6

[anthonyd]

sorry for the obnoxious stack trace.  i got some info:
apparently in nsBlockFrame, line 3315:

  while (nsnull != aLine->mNext) {
    rv = PullFrame(aState, aLine, &aLine->mNext, PR_FALSE, aDamageDeletedLines,
                   aFrameResult, stopPulling);
    if (NS_FAILED(rv) || stopPulling) {
      return rv;
    }
  }

a particular aLine can sometimes be added twice in a row to the list on 
nsLineBoxes.  what happens then, is that further down the line this:
nsBlockFrame line 3379
    aLine->SetChildCount(aLine->GetChildCount() + 1);

can be called more than once for a single nsLineBox, throwing off the children 
count.  Im not sure why, but I will add more as I find it.

adding buster and waterson to cc list.

anthonyd

Comment 7

[Kathleen :Brade]

changing from rtm+ to rtm NEED INFO (until patch is attached and reviewers have 
commented here)

Comment 8

[buster]

way back in the olden days of layout, floaters were handled by moving a frame
out-of-flow during reflow.  This was inefficient and error prone.  So we moved
floater handling into frame construction code where it belonged.  However, we
never updated the way content handled attribute change hints.  So when the
editor, or javascript, changes a content's align attribute, we were just asking
for a reflow when sometimes we really needed to ask for a reframe.  We need a
reframe whenever the align attribute is added or removed.  We can get by with a
much more efficient reflow when the align attribute is changed (from "left" to
"right" for example.)

I propose that to fix this crasher we need to change the content's hint for
align from "reflow" to "reframe".  As a performance enhancement post-rtm, we can
go back and be smarter about it, and only reframe when necessary.  This is a
rare operation, and the cost of reframing many objects (like images) is pretty
small.  However, some reframes can be expensive, like a table.

Comment 9

[anthonyd]

re-assigning to buster, as he seems to know *exactly* whats going on here, and 
its a layout issue.  I will gladly help in whatever way I can.

anthonyd

Comment 10

[buster]

changed summary to better describe what's going on.  This bug can occur in the
editor, or in any page that uses javascript to change the align attribute.

Comment 11

[buster]

Attachment: very low risk fix

Comment 12

[buster]

chris k, rick, or mark:  please review.
chris w, please double-secret super review.  see my previous comment for a
description of the fix.  I will add some appropriate commenting to the fix
before checking in.

Comment 13

[karnaze (gone)]

r=karnaze

Comment 14

[Chris Waterson]

- I think we need to worry about <input> and <iframe> for completeness
  sake. These can be floated as well.

- Also, not sure why <td> is affected: align on a <td> doesn't cause
  float; it causes justification, no?

Comment 15

[buster]

chris w: great catch.  new patch attached that handles iframe and input.
you misread the first patch.  <TD> isn't effected, it's <TABLE>.  <TABLE> is
floated with the HTML align attribute.

Comment 16

[buster]

Attachment: same diff, but also did iframe and input

Comment 17

[Chris Waterson]

Roger. Looks good, a=waterson

Comment 18

[buster]

marking rtm+ with karnaze's verbal blessing.  he reviewed this one for me
already, and waterson has approved the fix.

Very safe fix that increases both stability and correctness.  The code I changed
forces layout to use an existing, more conservative existing code path for the
case when the HTML "align" attribute is changed dynamically, as can happen with
the DOM or through the editor.  Since I'm just making use of a well-tested
codepath that already exists, the risk is very minimal.  There is a slight
performance hit (covered in bug 55967 -> Future), but this is an infrequent
operation, and the perf cost is on average very low.

Comment 19

[selmer (gone)]

rtm++

Comment 20

[buster]

fix checked into branch

Comment 21

[buster]

fixed into trunk as well

Comment 22

[sujay]

verified in 10/17 build.