Closed Bug 370174 Opened 16 years ago Closed 15 years ago

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


(Core :: Layout, defect, P3)






(Reporter: martijn.martijn, Assigned: uriber)



(Keywords: crash, regression, testcase, Whiteboard: [sg:nse null-deref] keep private until fixed for unminimized testcase[dbaron-1.9:RsCo])

Crash Data


(8 files, 1 obsolete file)

Attached file 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:
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]
Flags: blocking1.9?
Attached file testcase2
Much simpler testcase that also triggers this crash.
This one just removes all content in the page with document.documentElement.innerHTML = '';
Attachment #254831 - Attachment is private: true
Group: security
Attached patch patch?Splinter Review
Attachment #260714 - Flags: review?(sharparrow1)
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.
I'll probably only be able to have a look at this on Saturday.
Attached patch alternative patch (obsolete) — Splinter Review
This might be slightly better, as it treats the "content has no parent" case just like the "frame has no parent" one.
Attachment #260903 - Flags: review?(sharparrow1)
Comment on attachment 260903 [details] [diff] [review]
alternative patch

Okay, I think I like this better.
Attachment #260903 - Flags: review?(sharparrow1) → review+
Attachment #260714 - Flags: review?(sharparrow1)
Attachment #260903 - Flags: superreview?(roc)
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?
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.
Assignee: nobody → uriber
Attachment #260903 - Attachment is obsolete: true
Attachment #261305 - Flags: superreview?(roc)
Attachment #260903 - Flags: superreview?(roc)
Attachment #261305 - Flags: superreview?(roc) → superreview+
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
Closed: 16 years ago
Flags: blocking1.9?
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Flags: in-testsuite?
Thanks for the patch Uri, the simple testcase doesn't crash anymore, but the first, unminimised testcase is still crashing in current trunk build.
Group: security
Resolution: FIXED → ---
Attachment #254831 - Attachment is private: false
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:
(all of the following have the same content pointer!)
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.
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.
Whiteboard: [sg:nse null-deref] keep private until fixed for unminimized testcase
Attached file 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)
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Whiteboard: [sg:nse null-deref] keep private until fixed for unminimized testcase → [sg:nse null-deref] keep private until fixed for unminimized testcase[dbaron-1.9:RsCo]
Attached file 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.
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:

Roc, any chance you remember why you removed this?
That guard was added in bug 310589, BTW.
Probably just a mistake. Let's put it back, but I'd like to understand how we get into this state.
Attached patch add null-checkSplinter Review
This is the same as attachment 198443 [details] [diff] [review] (from bug 310589).
Attachment #305065 - Flags: superreview?(roc)
Attachment #305065 - Flags: review?(roc)
Attachment #305065 - Attachment description: add nuull-check → add null-check
Comment on attachment 305065 [details] [diff] [review]
add null-check

I swear I ticked the "patch" checkbox when I attached it.
Attachment #305065 - Attachment is patch: true
Attachment #305065 - Attachment mime type: application/octet-stream → text/plain
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...
Attachment #305065 - Flags: superreview?(roc)
Attachment #305065 - Flags: superreview+
Attachment #305065 - Flags: review?(roc)
Attachment #305065 - Flags: review+
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.
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.
Closed: 16 years ago15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
That plus an assertion would be good.
Checked in a followup with comment+assertion+initialization of result.mOffset.
Verified fixed with the attached testcases.
Crash Signature: [@ FindBlockFrameOrBR]
dveditz, I think we can open this bug now.  I don't see any sensitive information
here, including all testcases.
Group: core-security
You need to log in before you can comment on or make changes to this bug.