Last Comment Bug 369150 - Crash [@ nsHTMLFramesetFrame::GetNoResize] with dynamic changes
: Crash [@ nsHTMLFramesetFrame::GetNoResize] with dynamic changes
: crash, fixed1.8.1.4, testcase, topcrash, verified1.8.0.12
Product: Core
Classification: Components
Component: Layout: HTML Frames (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: 367471 (view as bug list)
Depends on:
Blocks: 369230 306660 367471 370504 372013
  Show dependency treegraph
Reported: 2007-02-02 16:16 PST by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
14 users (show)
dveditz: blocking1.8.1.4+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.12+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (crashes Firefox when loaded) (384 bytes, text/html)
2007-02-02 16:16 PST, Jesse Ruderman
no flags Details
possible patch (936 bytes, patch)
2007-02-06 12:57 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
testcase2 (480 bytes, text/html)
2007-03-21 08:51 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
just to show where the problem is (2.37 KB, patch)
2007-03-22 13:13 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
possible patch (1.60 KB, patch)
2007-03-22 13:51 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v2 (1.95 KB, patch)
2007-03-22 16:51 PDT, Olli Pettay [:smaug]
roc: review+
roc: superreview+
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review
set aDesiredSize.width and .height to 0 (1.23 KB, patch)
2007-04-10 10:00 PDT, Olli Pettay [:smaug]
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
well then this :) (1.29 KB, patch)
2007-04-10 11:26 PDT, Olli Pettay [:smaug]
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review

Description Jesse Ruderman 2007-02-02 16:16:34 PST
Created attachment 253805 [details]
testcase (crashes Firefox when loaded)

Loading the testcase makes Firefox crash [@ nsHTMLFramesetFrame::GetNoResize].
Comment 1 Olli Pettay [:smaug] 2007-02-06 12:57:14 PST
Created attachment 254195 [details] [diff] [review]
possible patch

I think we could just add a simple null check.
Comment 2 Boris Zbarsky [:bz] 2007-02-06 19:33:19 PST
Er.... but why are we ending up with an out-of-bounds index here?
Comment 3 Boris Zbarsky [:bz] 2007-02-06 19:52:43 PST
In particular, aren't we also reading past the bounds of the aChildTypes array, etc?
Comment 4 Olli Pettay [:smaug] 2007-02-07 02:18:40 PST
Comment on attachment 254195 [details] [diff] [review]
possible patch

True. There is something else going wrong.
The testcase seems to change mNumRows or mNumCols before NS_FRAME_FIRST_REFLOW.
Comment 5 Boris Zbarsky [:bz] 2007-02-07 09:33:09 PST
Well, there should be multiple NS_FRAME_FIRST_REFLOWs here -- appending a text node and changing the "rows" attribute like that should both trigger a frame reconstruct.
Comment 6 Mike Kaply [:mkaply] 2007-02-27 11:45:23 PST
*** Bug 367471 has been marked as a duplicate of this bug. ***
Comment 7 Mike Kaply [:mkaply] 2007-02-27 13:17:22 PST
We're seeing a "similar" frame problem:

crash is in a different place, but we are seeing it intermittently with this crash.
Comment 8 Daniel Veditz [:dveditz] 2007-02-27 23:25:49 PST
Crashes FF2.0.0.2 also, have not tested FF1.5.x
Comment 9 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-28 15:14:32 PST
(In reply to comment #2) (from bug 372013)
> Does bug 369150 have the same regression window?

doesn't crash in a 2005-09-05 build, does crash in a 2005-09-06 build.
Comment 10 Boris Zbarsky [:bz] 2007-02-28 15:20:10 PST
Ugh.  I'll look into this, I guess.  ;)
Comment 11 Daniel Veditz [:dveditz] 2007-03-07 13:52:28 PST
marking sg:critical based on comment 2 and 3
Comment 12 Window Snyder 2007-03-08 13:21:23 PST
Smaug: Can you take this bug?

Critical security bugs need to have an owner.  If you are not the correct person for this bug, please help us find someone else.  Thanks.
Comment 13 Olli Pettay [:smaug] 2007-03-08 13:29:19 PST
Well, Boris said already "I'll look into this, I guess. ;)"
Bz, do you have time for this? 
Comment 14 Boris Zbarsky [:bz] 2007-03-08 18:35:03 PST
The right owner is probably me; I'm just not sure I'll get a chance to debug it before March 27.  Between now and then I'm at home for about 90 hours all told.  :(

So if I don't get to it tomorrow and we need it fixed Soon, someone else would need to help...
Comment 15 Olli Pettay [:smaug] 2007-03-21 08:17:21 PDT
hmm, doesn't crash currently in linux-trunk.
Comment 17 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-03-21 08:51:23 PDT
Created attachment 259212 [details]

This crash still happens when appending a frame element instead of a text node.
Comment 18 Olli Pettay [:smaug] 2007-03-22 13:13:34 PDT
Created attachment 259350 [details] [diff] [review]
just to show where the problem is
Comment 19 Olli Pettay [:smaug] 2007-03-22 13:14:06 PDT
or not "where", but "what"
Comment 20 Olli Pettay [:smaug] 2007-03-22 13:51:22 PDT
Created attachment 259353 [details] [diff] [review]
possible patch

This is simple, though a bit ugly fix.
Better would be to make sure that the old frame isn't reflowed at all
if the number of cols or rows has changed, but I don't know 
how to do that in any elegant way. Flushing in SetAttr is not good enough (someone may use mutation events to do something evil).

The patch works also in branches (with --fuzz=4).

bz is still away, so maybe you roc can look at this?
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-03-22 16:37:19 PDT
I think this is a good approach.

Should we do the early exit if drag is active too? It seems to me that we should.
Comment 22 Olli Pettay [:smaug] 2007-03-22 16:51:11 PDT
Created attachment 259372 [details] [diff] [review]
Comment 23 Daniel Veditz [:dveditz] 2007-03-27 15:36:49 PDT
Comment on attachment 259372 [details] [diff] [review]

approved for, a=dveditz for release-drivers
Comment 24 Boris Zbarsky [:bz] 2007-03-28 23:25:50 PDT
Smaug, thanks for hunting this down!  Sorry I didn't get a chance to do that before I disappeared.  :(

I do have two comments about the patch:

1)  This patch assumes that the caller doesn't check the return value from Reflow().  If the caller does, we'll abort reflow of the caller and its other kids, possibly ending up in an inconsistent state.  If the caller doesn't, it'll ignore our retval and look at the reflow metrics.  So I would much prefer it if we'd set the reflow metrics to sane values (like 0 by 0 or whatnot, or whatever we set them to right now) and returned NS_OK.  I think we should make that change on both trunk and branches.

2)  The real problem is that reflowing off an event doesn't process pending restyles before reflowing, isn't it?  Perhaps we should simply change that instead and back out this change?  Or are we also hitting this via FlushPendingNotifications(Flush_OnlyReflow) calls too?  I would be happy to drop support for flushing only reflow, expecially since at the moment that already flushes out content anyway, just not style reresolves...  Maybe that's the approach we should take on trunk?  Or maybe file a followup bug to back this patch out once roc's compositor thing happens?
Comment 25 Boris Zbarsky [:bz] 2007-04-08 20:53:15 PDT
Smaug, could you please post a followup patch or bug about item 1 in comment 24?
Comment 26 Olli Pettay [:smaug] 2007-04-10 10:00:33 PDT
Created attachment 261150 [details] [diff] [review]
set aDesiredSize.width and .height to 0

Like this?
Comment 27 Boris Zbarsky [:bz] 2007-04-10 11:03:55 PDT
Hmm.  We should probably also set aStatus and call NS_FRAME_SET_TRUNCATION.  Note that aDesiredSize seems to be already set to the right size by this point, so we shouldn't need to munge that.
Comment 28 Boris Zbarsky [:bz] 2007-04-10 11:04:21 PDT
Comment on attachment 261150 [details] [diff] [review]
set aDesiredSize.width and .height to 0

See previous comment
Comment 29 Olli Pettay [:smaug] 2007-04-10 11:26:49 PDT
Created attachment 261171 [details] [diff] [review]
well then this :)

Also Unsetting mDrag, just like what is done after a normal reflow.
Comment 30 Boris Zbarsky [:bz] 2007-04-10 11:59:58 PDT
Comment on attachment 261171 [details] [diff] [review]
well then this :)

Looks good!
Comment 31 Daniel Veditz [:dveditz] 2007-04-12 11:56:42 PDT
Comment on attachment 261171 [details] [diff] [review]
well then this :)

approved for and, a=dveditz
Comment 32 Daniel Veditz [:dveditz] 2007-04-12 11:57:25 PDT
Removing branch-fixed keywords so we don't forget we have to land an additional tweak.
Comment 33 Olli Pettay [:smaug] 2007-04-12 12:27:51 PDT
Comment on attachment 261171 [details] [diff] [review]
well then this :)

Checked in to branches
Comment 34 Marcia Knous [:marcia - use ni] 2007-05-08 16:09:40 PDT
verified on the 1.8.0 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv: Gecko/20070508 Firefox/ No crash with the testcases. Adding branch verified keyword.
Comment 35 juan becerra [:juanb] 2007-05-08 16:12:12 PDT
Also verified on: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20070501 Firefox/, and 15012rc1 version as well
Comment 36 Jesse Ruderman 2007-12-16 17:40:13 PST
Crashtests checked in.

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