Last Comment Bug 467881 - Crash [@ IsBindingAncestor] with frameset, iframe, {ib}
: Crash [@ IsBindingAncestor] with frameset, iframe, {ib}
Status: VERIFIED FIXED
[sg:critical?] common fuzz blocker - ...
: assertion, crash, testcase, verified1.9.0.9, verified1.9.1
Product: Core
Classification: Components
Component: Layout: HTML Frames (show other bugs)
: Trunk
: All All
: P2 critical (vote)
: mozilla1.9.2a1
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
Depends on:
Blocks: 343943
  Show dependency treegraph
 
Reported: 2008-12-03 22:51 PST by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
14 users (show)
roc: blocking1.9.1+
dveditz: blocking1.9.0.9+
dveditz: wanted1.9.0.x+
samuel.sidler+old: wanted1.8.1.x-
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (946 bytes, text/html)
2008-12-03 22:51 PST, Jesse Ruderman
no flags Details
patch to stop using constrained heights for viewport reflow (3.22 KB, patch)
2009-01-25 10:40 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch to stop using constrained heights for viewport reflow (7.28 KB, patch)
2009-01-25 12:04 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
roc: review+
roc: superreview+
dveditz: approval1.9.0.9+
Details | Diff | Review

Description Jesse Ruderman 2008-12-03 22:51:01 PST
Created attachment 351325 [details]
testcase (crashes Firefox when loaded)

###!!! ASSERTION: reflow roots should never split: 'status == NS_FRAME_COMPLETE', file /Users/jruderman/central/layout/base/nsPresShell.cpp, line 6347

###!!! ASSERTION: frame not in line: 'line->Contains(aDeletedFrame)', file /Users/jruderman/central/layout/generic/nsBlockFrame.cpp, line 5385

Crash [@ IsBindingAncestor] trying to access memory at 0xdddddded.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-12-05 09:57:12 PST
Looks like we have destroyed frames in the frame tree.  That's really bad.  That second assertion is pretty much summing up why _that_ happens, and I bet the first one explains the second one.

roc, this seems like something up your alley.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-12-05 11:15:49 PST
What's the reflow root in question?
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-12-05 11:27:27 PST
ViewportFrame.
Comment 4 Jesse Ruderman 2008-12-08 20:43:09 PST
This bug shows up with a few different crash assertions and crash signatures, so it kinda gets in the way of fuzzing.
Comment 5 Daniel Holbert [:dholbert] 2008-12-12 11:15:12 PST
Crashes my Linux mozilla-central build, too, with the assertions mentioned in comment 0.
  Platform --> All/All
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-01-25 10:12:30 PST
Setting a breakpoint on nsBlockFrame::SetOverflowLines (it split because it had overflow lines) shows that we have overflow lines because aState.mBottomEdge was 0 in nsBlockFrame::PlaceLine (when it should have been NS_UNCONSTRAINEDSIZE).
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-01-25 10:25:06 PST
So I think the basic problem is something like this:
 * we often reflow the viewport with a constrained height, but we trust that its child scroll frame won't pass this further.
 * when we have a frameset, the viewport doesn't have a child scroll frame
 * we don't have anything to prevent mixing framesets with other things, or changing the structure at the top of the document when the frameset is gone

(Need to investigate all these points in a little more detail, though.)

What's happening here is that the constrained available height that we give the viewport is making its way all the way down the tree.


There are probably two ways to fix this:
 * stop using constrained available heights
 * maintain the frame tree invariants

We probably ought to do both.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-01-25 10:40:17 PST
Created attachment 358727 [details] [diff] [review]
patch to stop using constrained heights for viewport reflow

This seems to just work, although I need to do a good bit more testing.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-01-25 12:04:21 PST
Created attachment 358737 [details] [diff] [review]
patch to stop using constrained heights for viewport reflow

Needed a few additional changes to fix resizing.
Comment 10 Daniel Veditz [:dveditz] 2009-01-26 15:47:13 PST
This is a common fuzz blocker, raising the priority slightly (p2) to try to keep it in the release. Since it's mostly patched I hope I'm not overstepping here.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-01-29 15:53:42 PST
http://hg.mozilla.org/mozilla-central/rev/ea3adcdfa653
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-01-31 13:00:01 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5ce32275362a
Comment 13 Daniel Veditz [:dveditz] 2009-02-20 11:49:54 PST
Comment on attachment 358737 [details] [diff] [review]
patch to stop using constrained heights for viewport reflow

Approved for 1.9.0.8, a=dveditz for release-drivers
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-02-20 11:53:51 PST
I was sort of hoping we'd ship this in a 1.9.1 beta release to make sure it didn't break anything before we shipped it in a 1.9.0.* release, but I suppose we're probably ok based on nightly testing.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-02-22 19:23:13 PST
Checked in to CVS trunk for 1.9.0.8, 2009-02-22 19:21/22 -0800.
Comment 16 Henrik Skupin (:whimboo) 2009-03-11 08:53:19 PDT
Verified on OS X and Windows with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090310 Minefield/3.2a1pre ID:20090310044308

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090309 Shiretoko/3.1b4pre (.NET CLR 3.5.30729) ID:20090309034003
Comment 17 Al Billings [:abillings] 2009-03-19 16:34:22 PDT
The testcase doesn't crash 1.9.0.7 on OS X or Windows XP. Does this require a debug build or does the testcase not work on 1.9.0?
Comment 18 Al Billings [:abillings] 2009-03-31 17:41:02 PDT
(In reply to comment #17)
> The testcase doesn't crash 1.9.0.7 on OS X or Windows XP. Does this require a
> debug build or does the testcase not work on 1.9.0?

I don't suppose someone could reply to this since I asked this almost two weeks ago? :-)
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-03-31 17:51:08 PDT
I don't know.  You could see what happens in a 1.9.0.* debug build if you want (or try under valgrind).

Hard to give more details without knowing what you're trying to do.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-03-31 18:58:29 PDT
Al, the testcase crashes reliably in a debug build, where we mark deallocated memory with the 0xdddddddd pattern.  In a non-debug build, it will randomly either crash or not crash or let someone run arbitrary code depending on the values that happen to be in the deallocated memory.
Comment 21 Henrik Skupin (:whimboo) 2009-03-31 23:51:15 PDT
The testcase constantly crashes 01/30 Shiretoko builds on Windows and OS X. With the fix no more crashes happen with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 (.NET CLR 3.5.30729) ID:20090305152042
Comment 22 Samuel Sidler (old account; do not CC) 2009-04-01 16:12:57 PDT
Doesn't crash using Firefox 2.0.0.20.
Comment 23 Carsten Book [:Tomcat] 2009-04-02 16:16:42 PDT
verified fixed 1.9.0.9 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5;
en-US; rv:1.9.0.9pre) Gecko/2009040221 Firefox/3.0.9pre (debug build) + : Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.9pre) Gecko/2009040214 Minefield/3.0.9pre - no crash on testcase
Comment 24 Jesse Ruderman 2009-04-26 23:34:29 PDT
Crashtest added:

http://hg.mozilla.org/mozilla-central/rev/ea128fc02710

Note You need to log in before you can comment on or make changes to this bug.