134508 - crash during page load [@ DeletingFrameSubtree]

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Andrew Schultz]

originally reported in bug 129013
Mozilla crashes if you scroll around while the above URL is loading.  This bug
regressed sometime between build 2002032608 and 2002032808.  The crash occurs in
nsCSSFrameConstructor.cpp.

http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#9047

9047     if (IsFrameSpecial(aFrame)) {
9048       nsIFrame* specialSibling;
9049       GetSpecialSibling(aFrameManager, aFrame, &specialSibling);
9050       DeletingFrameSubtree(aPresContext, aPresShell, aFrameManager,
9051                            specialSibling);
9052     }

IsFrameSpecial is true, but GetSpeicalSibling sets specialSibling to null. 
Calling DeleteFrameSubtree crashes.

Comment 1

[Andrew Schultz]

Attachment: stacktrace

Comment 2

[Andrew Schultz]

Attachment: patch

throw an assertion and don't crash.

Comment 3

[Andrew Schultz]

With the patch, the window goes blank when bad stuff happens, but does not
crash.  When the page is fully loaded, everything is back to normal.  This is
the same behavior as before the regression (20020326).

Comment 4

[Igor Furlan]

I am NOT able to force mozilla to crash...  WFM

Mozilla 0.9.9
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.9) Gecko/20020310

RH6.2 kernel 2.2.14-5

Comment 5

[Andrew Schultz]

0.9.9 is pre-regression (20020326)...

It seems that the crash now occurs during load without scrolling (same build,
so I'm confused), but only if it loads slowly enough that Mozilla does reflow
during load.  I'm working on a testcase, but the crash seems dependent on
load speed (loading locally, or from my localhost web server works fine).

stacktrace is still the same.

Comment 6

[Andrew Schultz]

Attachment: testcase

crash seems to depend on lines containing

<a name="____"><em><li></em>"_______"</a>

scrolling increases the reproducibility of the crash, but is not always
required.

Comment 7

[Andrew Schultz]

also crashes with linux build 20020327 (RPM)
regression is between 2002032608 and 2002032709

Comment 8

[Christine Hoffman]

Changing QA contact

Comment 9

[Marc Attinasi]

I cannot duplicate the crash on the URL or the testcase, but the assertion /
check done in the patch looks reasonable to me anyway.

Comment 10

[Andrew Schultz]

I still crash with build 20020402.  The URL is easier to reproduce than the
testcase.  Neither may be reproducible with broadband.  I was unable to
reproduce the bug locally and even over my 56K modem, I downloaded Mozilla
source in the background to get the testcase to crash more reproducibly.

Comment 11

[Andrew Schultz]

here's a longer testcase:
http://turbo.che.ncsu.edu/andrew/134508_long.html

this is 40x as long (1.5MB of html) as the attached testcase and crashes for me
even loading locally.

Comment 12

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

This looks like it might be a regression from my changes for bug 129350.

Comment 13

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

Yes, I did cause this, because I made |DeletingFrameSubtree| no longer
null-safe.  I don't see why it should be null-safe, but it just happened to be
written that it was.

We shouldn't assert that a frame has a special sibling.  I think we should
assert that the argument to DeletingFrameSubtree is non-null, but for now I
think we should also check in release builds in case there are other callers
passing null.

I'll attach a revised patch and take the bug.

Comment 14

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

Attachment: revised patch

Comment 15

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

Attachment: one more tweak

One more tweak: now that we null-check at the beginning, change while-do to
do-while.  Also a diff -u10 for easier reading.

Comment 16

[Chris Waterson]

Comment on attachment 77914 [details] [diff] [review]
one more tweak

r=/sr=waterson

Comment 17

[Marc Attinasi]

Comment on attachment 77914 [details] [diff] [review]
one more tweak

r=attinasi

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 77914 [details] [diff] [review]
one more tweak

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 19

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

Fix checked in 2002-04-08 18:11 PDT.