369150 - Crash [@ nsHTMLFramesetFrame::GetNoResize] with dynamic changes

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase makes Firefox crash [@ nsHTMLFramesetFrame::GetNoResize].

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

I think we could just add a simple null check.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Er.... but why are we ending up with an out-of-bounds index here?

Comment 3

[Boris Zbarsky [:bzbarsky]]

In particular, aren't we also reading past the bounds of the aChildTypes array, etc?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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 [:bzbarsky]]

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 7

[Mike Kaply [:mkaply]]

We're seeing a "similar" frame problem:

https://bugzilla.mozilla.org/show_bug.cgi?id=372013

crash is in a different place, but we are seeing it intermittently with this crash.

Comment 8

[Daniel Veditz [:dveditz]]

Crashes FF2.0.0.2 also, have not tested FF1.5.x

Comment 9

[Martijn Wargers (dead)]

(In reply to comment #2) (from bug 372013)
> Does bug 369150 have the same regression window?

Yes.
doesn't crash in a 2005-09-05 build, does crash in a 2005-09-06 build.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ugh.  I'll look into this, I guess.  ;)

Comment 11

[Daniel Veditz [:dveditz]]

marking sg:critical based on comment 2 and 3

Comment 12

[Window Snyder]

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][[e-mail]]]

Well, Boris said already "I'll look into this, I guess. ;)"
Bz, do you have time for this? 

Comment 14

[Boris Zbarsky [:bzbarsky]]

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][[e-mail]]]

hmm, doesn't crash currently in linux-trunk.

Comment 16

[Martijn Wargers (dead)]

This seems to have been fixed between 2007-03-04 and 2007-03-05:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-03-04+04&maxdate=2007-03-05+09&cvsroot=%2Fcvsroot
Fixed by bug 370430, somehow?

Comment 17

[Martijn Wargers (dead)]

Attachment: testcase2

This crash still happens when appending a frame element instead of a text node.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: just to show where the problem is

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

or not "where", but "what"

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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) (email my personal email if necessary)]

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][[e-mail]]]

Attachment: v2

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 259372 [details] [diff] [review]
v2

approved for 1.8.0.12/1.8.1.4, a=dveditz for release-drivers

Comment 24

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Smaug, could you please post a followup patch or bug about item 1 in comment 24?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: set aDesiredSize.width and .height to 0

Like this?

Comment 27

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Comment on attachment 261150 [details] [diff] [review]
set aDesiredSize.width and .height to 0

See previous comment

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Attachment: well then this :)

Also Unsetting mDrag, just like what is done after a normal reflow.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 261171 [details] [diff] [review]
well then this :)

Looks good!

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 261171 [details] [diff] [review]
well then this :)

approved for 1.8.0.12 and 1.8.1.4, a=dveditz

Comment 32

[Daniel Veditz [:dveditz]]

Removing branch-fixed keywords so we don't forget we have to land an additional tweak.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 261171 [details] [diff] [review]
well then this :)

Checked in to branches

Comment 34

[Marcia Knous [:marcia]]

verified on the 1.8.0 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.12pre) Gecko/20070508 Firefox/1.5.0.12pre. No crash with the testcases. Adding branch verified keyword.

Comment 35

[juan becerra [:juanb]]

Also verified on: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070501 Firefox/2.0.0.4, and 15012rc1 version as well

Comment 36

[Jesse Ruderman]

Crashtests checked in.