268119 - Freeze/crash with [@ 0x0000189c nsBlockFrame::AddFrames] :first-letter inside absolute positioned div [@ nsCSSFrameConstructor::WrapFramesInFirstLetterFrame]

Status: VERIFIED FIXED

(Core :: Layout: Block and Inline, defect, P1)

Description

[Martijn Wargers (dead)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041106 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041106 Firefox/0.9.1+

See upcoming testcase.
The url did crash for me once, but is not really easy reproducable.

It seems a regression:
It doesn't happen with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041007
Firefox/0.9.1+ 7:21am
It happens with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041008
Firefox/0.9.1+ 6:12am

Bonsai link of that period:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-10-07+07%3A00%3A00&maxdate=2004-10-08+07%3A00%3A00&cvsroot=%2Fcvsroot

A Talkback report:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB1761978M



Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase (will crash when visiting)

I had to use Hyatt's hack (<script>var x=document.body.offsetHeight;</script>)
to trigger the freeze/crash.

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

Might be a DUP of bug 265899

Comment 3

[Boris Zbarsky [:bzbarsky]]

I see the following asserts before crashing:

###!!! ASSERTION: frame not in line: 'line->Contains(aDeletedFrame)', file
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 5025
###!!! ASSERTION: can't find deleted frame in lines: 'Error', file
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 5012

Sounds like blockframe gets confused about which frames belong where... roc, any
idea what's up here?

Comment 4

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

I'm not sure why it would be deleting a frame here.

Could be frame parenting confusion I guess.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

The problem seems to be that nsBlockFrame::DoRemoveFrame can't handle a
next-in-flow on the same line. The frame tree when DoRemoveFrame()
is called:

Block(div)(1)@0x86f3998 {0,0,8498,476} [state=20000004] sc=0x86f394c(i=2,b=0)<
  line 0x86f3c44: count=2 state=inline,clean,prevmarginclean,not
impacted,wrapped,nobr[0x1020] {0,0,8498,238} <
    Letter(0)@0x86f3be0 next=0x86fc5d0 next-in-flow=0x86fc5d0 {0,0,182,238}
[state=00000004] [content=0x86f5578] [sc=0x86f3b74] pst=:first-letter<
      Text(0)@0x86f3ba0[0,1,F]	next-in-flow=0x86e18c8 {0,0,182,238}
[state=08000004] sc=0x86f3c18 pst=:-moz-non-element<
	"W"
      >
    >
    Letter(0)@0x86fc5d0 next=0x86fc68c prev-in-flow=0x86f3be0
next-in-flow=0x86fc68c {182,0,8316,238} [state=00000004] [content=0x86f5578]
[sc=0x86f1248] pst=:-moz-non-element<
      Text(0)@0x86e18c8[1,97,F]  prev-in-flow=0x86f3ba0 next-in-flow=0x86fc648
{0,0,8316,238} [state=24000004] sc=0x86e1d0c pst=:-moz-non-element<
	"arning that film piracy represents the greatest economic threat to the
movie industry in its 100 "
      >
    >
  >
  line 0x86fc6c4: count=1 state=inline,clean,prevmarginclean,not impacted,not
wrapped,nobr[0x800] {0,238,1918,238} <
    Letter(0)@0x86fc68c prev-in-flow=0x86fc5d0 {0,238,1918,238}
[state=00000004] [content=0x86f5578] [sc=0x86f1248] pst=:-moz-non-element<
      Text(0)@0x86fc648[98,23,T]  prev-in-flow=0x86e18c8 {0,0,1918,238}
[state=60000004] sc=0x86e1d0c pst=:-moz-non-element<
	"year-plus history, the "
      >
    >
  >
>


After the first frame [0x86f3be0] is destroyed (successfully), we advance
to the next-in-flow [0x86fc5d0], but we also advance the 'line' iterator
to the next line [0x86fc6c4]. The 'line' is now out-of-sync so the
(destroyed) frame is not removed properly.

Comment 6

[Mats Palmgren (inactive)]

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

Comment 7

[Mats Palmgren (inactive)]

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

Comment 8

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

Were you planning to request reviews?

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 166027 [details] [diff] [review]
Patch rev. 1

OK, I was trying to figure out if this could occur with other
frame types as well, but I don't think so. I had an assertion
for it and ran some tests but it did not trigger.
How about form controls with some weird styling?

Comment 10

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

Comment on attachment 166027 [details] [diff] [review]
Patch rev. 1

sr=dbaron, but pushing review request to roc since he's much more familiar with
this code than I am.

Also, is |!line->Contains(aDeletedFrame)) ever true?  There's an assertion
earlier that it isn't (could it change?).

Comment 11

[Mats Palmgren (inactive)]

(In reply to comment #10)
> Also, is |!line->Contains(aDeletedFrame)) ever true?  There's an assertion
> earlier that it isn't (could it change?).


At this point |aDeletedFrame| is the next-in-flow of the one in the assertion.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 13

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

Comment on attachment 166027 [details] [diff] [review]
Patch rev. 1

Great catch, Mats.

+      if (!aDeletedFrame ||
+	   aDeletedFrame->GetType() != nsLayoutAtoms::letterFrame ||
+	   !line->Contains(aDeletedFrame)) 
Why can't this just be "if (aDeletedFrame && !line->Contains(aDeletedFrame))"?
The type check may speed things up a little --- Contains could be expensive ---
but I'm not sure that this is performance critical code.

I think the code that confused dbaron would be significantly clearer if we
renamed 'nextInFlow' to 'deletedNextInFlow', and used it throughout the loop
body instead of aDeletedFrame, only assigning it to aDeletedFrame at the end of
the loop body.

Comment 14

[Mats Palmgren (inactive)]

(In reply to comment #13)
> Why can't this just be "if (aDeletedFrame && !line->Contains(aDeletedFrame))"?

Yes, the type check was just an attempted optimisation... I tested this a bit.

About 60 page loads from the Top100 list + 1 page load of the testcase
gave 422 calls to 'DoRemoveFrame()' totally.
When we reach the added 'if'-statement we have:
  175: aDeletedFrame == NULL (there was no next-in-flow)
  1:   a letter frame on the same line (from the testcase)
  0:   a letter frame on some other line
  0:   a non-letter frame on the same line
  0:   a non-letter frame on some other line

So it appears the attempted optimisation was useless.

> I think the code that confused dbaron would be significantly clearer if we
> renamed 'nextInFlow' to 'deletedNextInFlow', and used it throughout the loop
> body instead of aDeletedFrame, only assigning it to aDeletedFrame at the end
> of the loop body.

I see your point, but the problem is that 'aDeletedFrame' is used after the
loop, so if the assignment is at the end it's easy to forget to update it
in case someone adds an early 'break' in the future?

I'm a bit unsure of the preferred coding style here -
do we prefer "if (nsnull != deletedNextInFlow)" over "if (deletedNextInFlow)"?
(personally, I find the latter much easier to read).

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Changes since rev. 1:
  1. removed the frame type check
  2. renamed 'nextInFlow' to 'deletedNextInFlow' and use it instead of
     'aDeletedFrame' -- but, I left the assignment to 'aDeletedFrame'
     where it was for now.
  3. a couple of optimisations at the end of the loop:
   A.  we call 'TryAllLines' to update the iterator even if 'deletedNextInFlow'

       is null which is unnecessary since the loop will terminate anyway.
   B.  I swapped the order of the "different parent" block and the
       "isLastFrameOnLine" block - again the 'TryAllLines' is unnecessary
       if we are breaking out of the loop.

Comment 16

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

Comment on attachment 167979 [details] [diff] [review]
Patch rev. 2

+    if (nsnull != deletedNextInFlow) {

"if (deletedNextInFlow)" is preferred. Looks good.

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

Isn't this ready to land?

Comment 18

[Mats Palmgren (inactive)]

(In reply to comment #16)
> "if (deletedNextInFlow)" is preferred.
> 

Filed bug 274329 on that.

Comment 19

[Mats Palmgren (inactive)]

Checked in 2004-12-12 10:15 PDT

-> FIXED

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED with testcase
https://bugzilla.mozilla.org/attachment.cgi?id=164874 using Mozilla/5.0
(Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041220