42372 - crash during frame destruction code while leaving page

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[joki (gone)]

This is a test case from bug #22636, an event handling bug of mine.  We had a 
lot of problems getting the parsing and event handling of this test case 
correct.  Now that all of them seem to work I'm getting a crash here.  However, 
at the time the parsing and event handling changes were made a month and a half 
back the test case still worked so I don't think the changes made to fix the bug 
are at issue.

Anyway, while attempting to verfiy I started running into a reproducible crash 
while clicking on the link in the test case running WinNT.  I'll attach a stack 
trace as well.

Comment 1

[joki (gone)]

Attachment: Stack trace

Comment 2

[leger]

Adding crash keyword

Comment 3

[buster]

Attachment: debug log, loading the page and dumping frames. Note the error in parentage. That's the reason for the crash.

Comment 4

[buster]

frame parentage issues can be tricky.  this is a pretty obscure case, an 
absolutely positioned element inside of a relatively positioned element.  could 
be a simple error, so it's worth a quick look.  but if it's not quick, I'd 
suggest *not* holding beta2 for this one, and fixing this for beta3 (when I get 
back.)

cc-ing waterson and karnaze in case either wants to look into it. 

marking "helpwanted" because it would be great if someone could build a little 
test suite of various combinations of positioned elements inside each other, to 
make sure we catch all the cases.

Comment 5

[ekrock's old account (dead)]

Crasher. PDT team please approve.

Comment 6

[Marc Attinasi]

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

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Minimal test case, 2 span elements, outer rel, inner abs pos

Comment 8

[Marc Attinasi]

Taking a stab at prioritizing buster's nsbeta3 bugs...

Comment 9

[Marc Attinasi]

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

Comment 10

[Marc Attinasi]

This crash occurs on www.crn.com, and the page layout is really awful too 
(tabbed DIV contents are not contained correctly, general mucking up of abs and 
rel positioning) - see bug 47200.

Comment 11

[Marc Attinasi]

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

Comment 12

[buster]

staring at the code....

Comment 13

[buster]

Interesting....this only happens if the outer tag is an inline.  Some 
block-in-inline badness, no doubt.  Still researching....

Comment 14

[buster]

I think I have this fixed.  Patch coming soon.
There were 2 basic problems.  First, the frame and view trees were not being
updated together when a frame was reparented.  Second, a subtle bug in the use
of a stack-based variable was causing the frame parentage to be wrong in some
cases, when the variable's destructor was prematurely fired. 

Comment 15

[buster]

Attachment: proposed patch

Comment 16

[Chris Waterson]

Which code is relevant to this bug in the patch you've attached? (This patch
also appears to include some image debugging printf's and the text edit initial
reflow stuff...)

Comment 17

[Marc Attinasi]

From Steve's email it looks like nsCSSFrameConstructor.cpp and 
nsHTMLReflowState.cpp are the changes for this bug. If those are the important 
changes, then they look reasonable to my (bloodshot) eyes, but I have not run 
'em.

Comment 18

[buster]

damn, sorry, I attached the wrong patch file.  I had hand-edited a concise patch 
file, but that one didn't make it.  When I get to the office, I'll do it right.

Comment 19

[buster]

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

Comment 20

[buster]

Attachment: here's the real patch

Comment 21

[buster]

turns out the part of the fix for the view tree was a red herring.  with the 
other part of the fix (in nsCSSFrameConstructor::ConstructInline()), the view 
tree stays in synch without the additional code.  However, I think that code 
could be useful, if only I could find a test case that triggers the problem!  
So, I'm commenting it out but leaving it in the file with a comment about what 
to look for and how it could be useful.

Comment 22

[buster]

r=waterson

Comment 23

[Chris Petersen]

Steve,

What's the best way I can verify this problem has been fixed ?

Comment 24

[buster]

to verify:
1. load the test case
2. load any other page, or just hit reload
3. if you crash, it ain't fixed

Comment 25

[Chris Petersen]

No longer crashing. Tested with the Aug 24th build.

Comment 26

[buster]

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