Crash [@ nsHTMLFramesetFrame::GetNoResize] with dynamic changes

RESOLVED FIXED

Status

()

Core
Layout: HTML Frames
--
critical
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: smaug)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
x86
Mac OS X
crash, fixed1.8.1.4, testcase, topcrash, verified1.8.0.12
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.4 +
wanted1.8.1.x +
blocking1.8.0.12 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
Created attachment 253805 [details]
testcase (crashes Firefox when loaded)

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

Updated

11 years ago
Severity: normal → critical
(Reporter)

Updated

11 years ago
Blocks: 369230
(Assignee)

Comment 1

11 years ago
Created attachment 254195 [details] [diff] [review]
possible patch

I think we could just add a simple null check.
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #254195 - Flags: superreview?(roc)
Attachment #254195 - Flags: review?(roc)
Er.... but why are we ending up with an out-of-bounds index here?
In particular, aren't we also reading past the bounds of the aChildTypes array, etc?
(Assignee)

Comment 4

11 years ago
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.
Attachment #254195 - Flags: superreview?(roc)
Attachment #254195 - Flags: review?(roc)
(Assignee)

Updated

11 years ago
Assignee: Olli.Pettay → nobody
Status: ASSIGNED → NEW
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.
Flags: blocking1.9?
Blocks: 370504

Updated

11 years ago
Duplicate of this bug: 367471

Updated

11 years ago
Keywords: topcrash

Comment 7

11 years ago
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.
Blocks: 372013
Crashes FF2.0.0.2 also, have not tested FF1.5.x
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
(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.
Ugh.  I'll look into this, I guess.  ;)
Blocks: 306660
marking sg:critical based on comment 2 and 3
Group: security
Whiteboard: [sg:critical]

Comment 12

11 years ago
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.
Assignee: nobody → Olli.Pettay
(Assignee)

Comment 13

11 years ago
Well, Boris said already "I'll look into this, I guess. ;)"
Bz, do you have time for this? 
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...
(Assignee)

Comment 15

11 years ago
hmm, doesn't crash currently in linux-trunk.
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?
Created attachment 259212 [details]
testcase2

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

Updated

11 years ago
Assignee: Olli.Pettay → bzbarsky
(Assignee)

Comment 18

11 years ago
Created attachment 259350 [details] [diff] [review]
just to show where the problem is
(Assignee)

Comment 19

11 years ago
or not "where", but "what"
(Assignee)

Comment 20

11 years ago
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?
Assignee: bzbarsky → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #259353 - Flags: review?(roc)
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.
(Assignee)

Comment 22

11 years ago
Created attachment 259372 [details] [diff] [review]
v2
Attachment #259353 - Attachment is obsolete: true
Attachment #259372 - Flags: review?(roc)
Attachment #259353 - Flags: review?(roc)
Attachment #259372 - Flags: superreview+
Attachment #259372 - Flags: review?(roc)
Attachment #259372 - Flags: review+
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Attachment #259372 - Flags: approval1.8.1.4?
Attachment #259372 - Flags: approval1.8.0.12?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12+
Comment on attachment 259372 [details] [diff] [review]
v2

approved for 1.8.0.12/1.8.1.4, a=dveditz for release-drivers
Attachment #259372 - Flags: approval1.8.1.4?
Attachment #259372 - Flags: approval1.8.1.4+
Attachment #259372 - Flags: approval1.8.0.12?
Attachment #259372 - Flags: approval1.8.0.12+
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.12, fixed1.8.1.4
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?
Smaug, could you please post a followup patch or bug about item 1 in comment 24?
(Assignee)

Comment 26

11 years ago
Created attachment 261150 [details] [diff] [review]
set aDesiredSize.width and .height to 0

Like this?
Attachment #261150 - Flags: superreview?(bzbarsky)
Attachment #261150 - Flags: review?(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 on attachment 261150 [details] [diff] [review]
set aDesiredSize.width and .height to 0

See previous comment
Attachment #261150 - Flags: superreview?(bzbarsky)
Attachment #261150 - Flags: superreview-
Attachment #261150 - Flags: review?(bzbarsky)
Attachment #261150 - Flags: review-
(Assignee)

Comment 29

11 years ago
Created attachment 261171 [details] [diff] [review]
well then this :)

Also Unsetting mDrag, just like what is done after a normal reflow.
Attachment #261171 - Flags: superreview?(bzbarsky)
Attachment #261171 - Flags: review?(bzbarsky)
Comment on attachment 261171 [details] [diff] [review]
well then this :)

Looks good!
Attachment #261171 - Flags: superreview?(bzbarsky)
Attachment #261171 - Flags: superreview+
Attachment #261171 - Flags: review?(bzbarsky)
Attachment #261171 - Flags: review+
(Assignee)

Updated

11 years ago
Attachment #261171 - Flags: approval1.8.1.4?
Attachment #261171 - Flags: approval1.8.0.12?
Comment on attachment 261171 [details] [diff] [review]
well then this :)

approved for 1.8.0.12 and 1.8.1.4, a=dveditz
Attachment #261171 - Flags: approval1.8.1.4?
Attachment #261171 - Flags: approval1.8.1.4+
Attachment #261171 - Flags: approval1.8.0.12?
Attachment #261171 - Flags: approval1.8.0.12+
Removing branch-fixed keywords so we don't forget we have to land an additional tweak.
Keywords: fixed1.8.0.12, fixed1.8.1.4
(Assignee)

Comment 33

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

Checked in to branches

Updated

11 years ago
Keywords: fixed1.8.0.12, fixed1.8.1.4
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.
Keywords: fixed1.8.0.12 → verified1.8.0.12
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
Group: security
Flags: wanted1.8.0.x?
Flags: in-testsuite?
Flags: blocking1.9?
(Reporter)

Comment 36

10 years ago
Crashtests checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsHTMLFramesetFrame::GetNoResize]
You need to log in before you can comment on or make changes to this bug.