134011 - {ib} page crashes browser

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P1)

Description

[Tormod V]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:0.9.9+) Gecko/20020326
BuildID:    2002032608

The above URL crashes the browser after displaying it. If javascript is
disabled, it does not crash.

Reproducible: Always
Steps to Reproduce:
1. Go to the URL, or go to http://www.davos.ch (which does not crash) then click
the "News" link on the right.


Actual Results:  Page loads, displays and mozilla dies.

Expected Results:  Page loads and displays.

Talkback TB4571918M. I have tried to isolate the bad js code by editing a local
copy of the html. If I disable the "shForm" in the body onload, and the loading
of "HM_Loader.js", it does not crash. But now I am too tired...

Comment 1

[Phil Schwartau]

Confirming crash with Mozilla trunk debug build 2002-03-24 WinNT.
OS: Mac9.x ---> All.

Will attach stack trace below -

Comment 2

[Phil Schwartau]

Attachment: WinNT stack trace

Comment 3

[Phil Schwartau]

Stack trace looks like it has to do with DOM CSS; therefore 
reassigning to DOM Style component for further triage -

Comment 4

[Johnny Stenback (:jst)]

The page is changing visibility on a "ilayer" tag, and that ends up doing a
stylechange reflow which ends up finding a reference to a deleted frame in the
frame tree and we crash trying to call QI on the deleted frame.

Over to layout.

Comment 5

[Hixie (not reading bugmail)]

Attachment: Testcase: click to crash

This is based on a crasher page I was given. Talkback ID TB4771799W. If the
stack trace matches then this is the bug -- the descriptions sounds similar.

Comment 6

[Marc Attinasi]

Thanks Chris!

Comment 7

[Chris Waterson]

I wasn't able to crash with today's build using the test case; however, the
original URL still crashes after the page has loaded.

Comment 8

[Chris Waterson]

Attachment: revised testcase, click on content area to crash.

Comment 9

[Chris Waterson]

Mmm, more block-in-inline fun. With an <hr> in the third part of the spit,
changing the visibility on the split hierarchy causes a crash.

Comment 10

[Chris Waterson]

Attachment: proposed fix, of the wallpaper variety.

Boy, we really need to re-do {ib}.

So here's what's happening. The <span> containing the <div> is being targeted
for a style change; specifically, the visibility is being changed via the style
DOM. We walk the {ib} siblings to propagate the style changes to those frames
as well; inside one of the siblings, a frame change is triggered for one of the
children (who knows why -- maybe this is another bug). Regardless, this has the
effect of adding the child to the change list with, but not updating the
top-level maxHint. When we ProcessRestyledFrames, the frame that wants a
reframe triggers the ReframeContainingBlock logic: this nukes _all_ the other
frames in the change list. So, when we try to process them, they've been
deleted, and we crash.

The fix is to inspect the change list after adding each of the {ib} siblings.
If we ever get into a situation where someone is going to get a frame change,
we bail, and reframe the whole kit and kaboodle.

Comment 11

[Chris Waterson]

Review-shopping for some {ib} spackle...

Comment 12

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

So this really seems quite wall-papery, considering AttributeChanged is by no
means the only caller of ComputeStyleChangeFor.  It also seems a bit too
agressive (since we really only need to do this if it's something that causes
|WipeContainingBlock| to be called, i.e., a frame that IS_FRAME_SPECIAL,
right?).  Wouldn't a cleaner fix be to move the |ReframeContainingBlock| logic
into the style change mechanism somehow, so that we send the reframe to the top
frame to begin with?  But r=dbaron if you want.

Now that you mention it, there probably is an easier way to rewrite {ib} stuff...

Comment 13

[Chris Waterson]

Ugh, I didn't look at all the other callers of ComputeStyleChangeFor. So you're
right, this is going to stop one kind of crash, but not four (or more?) others.
I'll see if I can fold the necessary {ib} logic into
nsFrameManager::CopmuteStyleChangeFor and ReResolveStyleContext.

Marc was right. This summer we ought to try to re-think this whole mess.

Comment 14

[Chris Petersen]

Reporter,

Please try with a new nighly build I can reproduce the crash on site or reduced
test case provided. Tested under OSX with 2002-07-31-05 1.0.

Comment 15

[Tormod V]

WFM :) Does not crash using the URL and the testcases, using mozilla-mac-trunk
Build ID 2002080108.