370174 - Crash [@ FindBlockFrameOrBR] with unminimised testcase triple-clicking at the bottom of the page

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Martijn Wargers (dead)]

Attachment: testcase

Marking security sensitive for now, because the testcase isn't minimised (I haven't been able to minimise it yet).

To reproduce the crash:
- Open testcase, wait >=2 seconds to let the style cycling finish (which is finished when 71:10 is reached).
- Triple-click somewhere on the page under the text

Result: crash

This seems to have regressed between 2006-02-03 and 2006-02-04:
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=2006-02-03+05&maxdate=2006-02-04+06&cvsroot=%2Fcvsroot
It seems to me this is a regression of bug 253076, somehow.

Talkback ID: TB29260735Q
FindBlockFrameOrBR  [mozilla\layout\generic\nsframe.cpp, line 4627]
FindBlockFrameOrBR  [mozilla\layout\generic\nsframe.cpp, line 4646]
FindBlockFrameOrBR  [mozilla\layout\generic\nsframe.cpp, line 4646]
FindBlockFrameOrBR  [mozilla\layout\generic\nsframe.cpp, line 4646]
FindBlockFrameOrBR  [mozilla\layout\generic\nsframe.cpp, line 4646]
FindBlockFrameOrBR  [mozilla\layout\generic\nsframe.cpp, line 4646]
nsIFrame::PeekOffsetParagraph  [mozilla\layout\generic\nsframe.cpp, line 4683]
nsIFrame::PeekOffset  [mozilla\layout\generic\nsframe.cpp, line 4952]
nsFrame::PeekBackwardAndForward  [mozilla\layout\generic\nsframe.cpp, line 2168]

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase2

Much simpler testcase that also triggers this crash.
This one just removes all content in the page with document.documentElement.innerHTML = '';

Comment 2

[Martijn Wargers (dead)]

Attachment: backtrace from debug build

Comment 3

[Martijn Wargers (dead)]

Attachment: patch?

Comment 4

[Eli Friedman]

I'm not completely sure, but I think the root content should get special handling instead of just failing.

CCing Uri for an opinion, since he wrote the relevant of code.

Comment 5

[Uri Bernstein (Google)]

I'll probably only be able to have a look at this on Saturday.

Comment 6

[Uri Bernstein (Google)]

Attachment: alternative patch

This might be slightly better, as it treats the "content has no parent" case just like the "frame has no parent" one.

Comment 7

[Eli Friedman]

Comment on attachment 260903 [details] [diff] [review]
alternative patch

Okay, I think I like this better.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why bother testing parent at all? A frame with no parent can't have parent content.

Don't some frames (pages?) have no content? Could this crash them?

Comment 9

[Uri Bernstein (Google)]

Attachment: alt patch, take 2

Don't test for a null parent, but do test for null content.

I tested this with the testcase on this bug, and with the testcases on bug 306049, and it doesn't crash in any of them.

Comment 10

[Uri Bernstein (Google)]

Checked in.

Checking in mozilla/layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.721; previous revision: 3.720
done

Comment 11

[Martijn Wargers (dead)]

Thanks for the patch Uri, the simple testcase doesn't crash anymore, but the first, unminimised testcase is still crashing in current trunk build.

Comment 12

[Eli Friedman]

Hmm, first off, this is not an exploitable crash; it's a null pointer dereference.  It's possible that you could get your testcase to crash in other places, though.

I'm getting what should be an impossible frame tree:
ViewportFrame
HTMLScrollFrame
(all of the following have the same content pointer!)
CanvasFrame
AreaFrame
TableOuterFrame
TableFrame
TableRowGroupFrame
TableRowFrame
TableCellFrame
BlockFrame (the frame it crashes on)

FindBlockFrameOrBR shouldn't crash under normal conditions because it's normally impossible for a block to have a block descendant with the same content pointer  However, with this wacky frame tree, that condition is no longer met.

I'm not really sure how dangerous a valid, but incorrect frame tree is, but it seems like some code could get confused and crash in more dangerous ways.

Not sure who to CC for a frame constructor bug.

Comment 13

[Martijn Wargers (dead)]

I'm made this bug security sensitive again, because the first testcase is not minimised. If the testcase was kept private, then you and Uri couldn't see the testcase.

Comment 14

[Martijn Wargers (dead)]

Attachment: testcase3

Ok, I finally have minimised the first testcase. This still crashes when you triple-click at the bottom of the page.
(you don't need to triple-click to crash when downloading the testcase to your computer, it has a script that does the triple-clicking for you there)

Comment 15

[Martijn Wargers (dead)]

Attachment: testcase4

I'm getting this crash too with this testcase, you need to download it to your computer, because of the use of enhanced privileges. Then you need to do some clicking in the iframe document without clicking on the embed.

Comment 16

[Uri Bernstein (Google)]

So what I'm getting here is a parentless content:
#0  0x1061122c in FindBlockFrameOrBR (aFrame=0x245514c, aDirection=eDirPrevious) at /Users/uri/mozilla/trunk/mozilla/layout/generic/nsFrame.cpp:4544
4544          (aDirection == eDirPrevious ? 1 : 0);
(gdb) p aFrame
$23 = (nsAreaFrame *) 0x245514c
(gdb) p content
$24 = (nsHTMLAnchorElement *) 0x362eb020
(gdb) p content->GetParent()
$25 = (Cannot access memory at address 0x0

This seems like something that shouldn't happen, but yet there used to be a |if (result.mContent)| guard checking for this, which was removed by roc in bug 317375:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/generic&command=DIFF&root=/cvsroot&file=nsFrame.cpp&rev1=3.609&rev2=3.610#29

Roc, any chance you remember why you removed this?

Comment 17

[Uri Bernstein (Google)]

That guard was added in bug 310589, BTW.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Probably just a mistake. Let's put it back, but I'd like to understand how we get into this state.

Comment 19

[Uri Bernstein (Google)]

Attachment: add null-check

This is the same as attachment 198443 [details] [diff] [review] (from bug 310589).

Comment 20

[Uri Bernstein (Google)]

Comment on attachment 305065 [details] [diff] [review]
add null-check

I swear I ticked the "patch" checkbox when I attached it.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 305065 [details] [diff] [review]
add null-check

sorry about removing that :-(. How about adding a comment explaining why it's needed? Preferably one that actually explains what's going on in these bugs...

Comment 22

[Uri Bernstein (Google)]

Oops. I saw your approval and checked this in before noticing your comment. I'll do a follow-up checkin with a comment. Would it also be useful to add an ASSERT there, so it would be easy to open a follow-up bug on it? Understanding what's actually going on here (i.e., why we get into this situation) is something I'm afraid I don't have the bandwidth to investigate right now.

Comment 23

[Uri Bernstein (Google)]

Resolving as FIXED for now. I'll check in the following comment:

    // In some cases (bug 310589, bug 370174) we end up here with a null content.
    // This probably shouldn't ever happen, but since it sometimes does, we want
    // to avoid crashing here.  

once I get your response regarding adding an assertion.

Also, I noticed that the fix I checked in is not only identical to attachment 198443 [details] [diff] [review], but also to attachment 260714 [details] [diff] [review] from this bug (it's deja vu all over again). I notice that in that patch I also initialized result.mOffset to 0, which is probably not a bad idea, so I'll throw that in with the follow-up patch if you don't mind.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That plus an assertion would be good.

Comment 25

[Uri Bernstein (Google)]

Checked in a followup with comment+assertion+initialization of result.mOffset.

Comment 26

[Martijn Wargers (dead)]

Verified fixed with the attached testcases.

Comment 27

[Mats Palmgren (inactive)]

dveditz, I think we can open this bug now.  I don't see any sensitive information
here, including all testcases.

Comment 28

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce53ea4d732d

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ce53ea4d732d