56563 - crash opening page

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[buster]

spun off from bug 53935
[e-mail] reports a crash at this page on branch only, all platforms.  
Will attach talkback reports.

Comment 1

[buster]

see talkback traces from bug 53935:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16953
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16954

Comment 2

[Blake Ross]

It's not branch-only; I crash with the same stack in a new win98 trunk build.

The assertion, btw, is:

###!!! ASSERTION: can't find deleted frame in lines: 'nsnull != line', file C:\m
ozilla\layout\html\base\src\nsBlockFrame.cpp, line 5461

should fix for RTM

Comment 3

[buster]

investigating...

Comment 4

[buster]

Attachment: minimized test case

Comment 5

[buster]

karnaze and harish:  Rick and I are working on this crasher.  It looks like the 
crash is coming about because the content and/or frame model are corrupt at the 
time we do an incremental reflow.  See the test case (it's hideous mark-up, but 
at least it's small.)  Rick thinks the code that moves content out of an 
illegally-formed table isn't doing the right thing.  Harish could look at 
that.  Secondly, given the incorrect content model, I think Chris' code in 
nsCSSFrameConstructor might not be doing the right thing either.  That should 
also get fixed, because this same sort of thing could have happened through the 
DOM (maybe...can the DOM create unknown tags?)

Comment 6

[rickg]

Here's the content model:

docshell=01089E90
html@02861938 refcount=6<
  head@02861848 refcount=2<
  >
  Text@028A2C50 refcount=3<\n>
  body@028A30B8 refcount=3<
    table@028D1A18 refcount=10<
      Text@028D2820 refcount=2<\n>
      sh@028D3928 _moz-userdefined= refcount=5<
        sh@028D4A48 _moz-userdefined= refcount=5<
          Text@028D4920 refcount=3<\n>
          p@028D5B68 refcount=3<
            Text@028D5A90 refcount=3<steve\n>
          >
          p@028D5968 refcount=3<
            Text@028D5890 refcount=3<\n>
            img@028D84FC src=http://www.inktomi.com/logo/PowByInkMed.gif 
height=40 width=126 refcount=3<>
            Text@028D9E00 refcount=3<\n>
          >
        >
      >
    >
    p@028D3628 refcount=3<
      Text@028D34F0 refcount=3<  \n>
    >
  >
>

I'll add the frame model as an attachment.

Comment 7

[rickg]

Attachment: frame model dump

Comment 8

[buster]

an odd thing about this bug:  on rick's machine, it crashes when viewer or 
mozilla are run normally, but does *not* crash when we step through it in the 
debugger. rick captured the frame model after running through the debugger with 
break points in the parser.  I don't think this is the interesting part of the 
problem.  In the crash case, it crashes during an incremental reflow (the image 
load, probably).  The debugger might change the timing of when the image is 
loaded. I think we need to capture the frame model earlier, before the 
incremental reflow comes through.

Comment 9

[buster]

Attachment: the frame model before the incremental reflow, manually dumped out by adding a call to List in nsTableOuterFrame::Reflow().

Comment 10

[buster]

the attachment I just added shows the screwy frame model just before we do the 
reflow that causes the crash.  karnaze: you and I should look at this together 
asap.  harish: you and rick should talk about the content model that gets 
created as a result of the test case.  

Comment 11

[buster]

I think the *primary* bug here, the one that's causing this crash, is the 
incorrect content model.  The table frame construction code is handling the 
illegal content model poorly, but that is a separate problem.  I'm going to 
assign this bug to harish so he can get the content model straightened out.  
I'll open a new bug on Chris, because even after Harish's fix an author could 
still get into trouble manipulating the document via the DOM.

Harish, it looks like it's the existance of the unknown tags <sh> that cause 
your illegal-content-in-table code to do the wrong thing.

Comment 12

[buster]

submitted bug 57010 for the frame portion of this bug.

Comment 13

[harishd]

At some point there was a consensus that userdefined tags could exist any where
in the document and therefore it wasn't necessary for me to treat them as
illegal-content inside table.

But, since the table code couldn't deal with userdefined tags we can definitely
make an exception, in the DTD, and throw them out of the table. Now, the
question is, should we deal this problem for rtm?

Comment 14

[rickg]

Tables *should* be able to handle this, but dont -- and we crash. That's really 
bad, and doable in both html source and via the DOM. I think this is a mandatory 
fix.

Comment 15

[harishd]

I agree that it's a crasher and should be fixed. But, we should know how many
sites are actully affected by this. Without an estimate it will be pretty hard
to pass thro' PDT.

Comment 16

[harishd]

Attachment: Proposed patch [ pertaining to table elements only ]

Comment 17

[harishd]

Attachment: Proposed patch [ // kBadContentWatch does not include TD ] - changed the comment.

Comment 18

[karnaze (gone)]

Harish, I'm going to work on a patch in table frame construction which may be 
safer than your patch. Please don't mark this rtm+ until tomorrow

Comment 19

[karnaze (gone)]

Attachment: frame construction patch

Comment 20

[karnaze (gone)]

The frame construction patch is a bit larger than I had anticipated. It still 
may be safer than the parser patch (we need to decide) and it is certainly more 
general (it will probably handle additional javascript cases). Neither the test 
case nor the url crashes, but the url is a very large page, and I am getting a 
ton of assertions in block code when scrolling, so I can't tell if it is 
rendering correctly.

Comment 21

[buster]

yes, it is a huge page.  But I was not getting any assertions while loading it,
while I was trying to minimize the test case.  In fact, you can verify this by
simply removing the image at the end of the document, thus removing the
incremental reflow and preventing the crash.

Comment 22

[karnaze (gone)]

With the frame construction patch more frames are getting constructed, so it is 
not too surprising that I am seeing new assertions. If we decide to go with this 
patch, then we need to get together and understand what is causing the 
assertions.

Comment 23

[Nisheeth Ranjan]

Unless we can have a less risky, smaller fix for this bug AND prove that this
bug occurs on high visibility sites, there is no way the PDT will allow this fix
into the rtm branch.  Based on current information, I don't think this bug is
high impact enough to warrant fixing it on the rtm branch.

Marking rtm-.  Please re-nominate if you disagree.

Comment 24

[Kurt Weinschenker]

Late in the game, but attaching MacsBug stdlog from crash on this bug.

Mac OS 9.0.4, Mac trunk Mozilla installer build 2000102312.

Comment 25

[Kurt Weinschenker]

Attachment: MacsBug stdlog from 10/16 testcase crash

Comment 26

[karnaze (gone)]

Attachment: revised frame construction patch (passes regresssion tests)

Comment 27

[karnaze (gone)]

Reassigning to myself.

Comment 28

[karnaze (gone)]

The frame construction patch has been checked in.

Comment 29

[Chris Petersen]

Verified in the 2001010209 build.