Closed
Bug 370174
Opened 16 years ago
Closed 15 years ago
Crash [@ FindBlockFrameOrBR] with unminimised testcase triple-clicking at the bottom of the page
Categories
(Core :: Layout, defect, P3)
Core
Layout
Tracking
()
VERIFIED
FIXED
mozilla1.9beta4
People
(Reporter: martijn.martijn, Assigned: uriber)
References
Details
(Keywords: crash, regression, testcase, Whiteboard: [sg:nse null-deref] keep private until fixed for unminimized testcase[dbaron-1.9:RsCo])
Crash Data
Attachments
(8 files, 1 obsolete file)
17.23 KB,
text/html
|
Details | |
284 bytes,
text/html
|
Details | |
4.36 KB,
text/plain
|
Details | |
2.07 KB,
patch
|
Details | Diff | Splinter Review | |
2.18 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
929 bytes,
text/html
|
Details | |
1.12 KB,
text/html
|
Details | |
1.44 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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]
Reporter | ||
Updated•16 years ago
|
Flags: blocking1.9?
Reporter | ||
Comment 1•16 years ago
|
||
Much simpler testcase that also triggers this crash. This one just removes all content in the page with document.documentElement.innerHTML = '';
Reporter | ||
Updated•16 years ago
|
Attachment #254831 -
Attachment is private: true
Reporter | ||
Updated•16 years ago
|
Group: security
Reporter | ||
Comment 2•16 years ago
|
||
Reporter | ||
Comment 3•16 years ago
|
||
Attachment #260714 -
Flags: review?(sharparrow1)
Comment 4•16 years ago
|
||
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.
Assignee | ||
Comment 5•16 years ago
|
||
I'll probably only be able to have a look at this on Saturday.
Assignee | ||
Comment 6•15 years ago
|
||
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 7•15 years ago
|
||
Comment on attachment 260903 [details] [diff] [review] alternative patch Okay, I think I like this better.
Attachment #260903 -
Flags: review?(sharparrow1) → review+
Updated•15 years ago
|
Attachment #260714 -
Flags: review?(sharparrow1)
Assignee | ||
Updated•15 years ago
|
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?
Assignee | ||
Comment 9•15 years ago
|
||
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
Status: NEW → ASSIGNED
Attachment #261305 -
Flags: superreview?(roc)
Attachment #260903 -
Flags: superreview?(roc)
Attachment #261305 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 10•15 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: blocking1.9?
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Updated•15 years ago
|
Flags: in-testsuite?
Reporter | ||
Comment 11•15 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Updated•15 years ago
|
Attachment #254831 -
Attachment is private: false
Comment 12•15 years ago
|
||
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.
Reporter | ||
Comment 13•15 years ago
|
||
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.
Updated•15 years ago
|
Whiteboard: [sg:nse null-deref] keep private until fixed for unminimized testcase
Reporter | ||
Comment 14•15 years ago
|
||
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)
Reporter | ||
Updated•15 years ago
|
Flags: blocking1.9?
Updated•15 years ago
|
Flags: blocking1.9? → blocking1.9+
Updated•15 years ago
|
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]
Priority: -- → P3
Reporter | ||
Comment 15•15 years ago
|
||
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.
Assignee | ||
Comment 16•15 years ago
|
||
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?
Assignee | ||
Comment 17•15 years ago
|
||
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.
Assignee | ||
Comment 19•15 years ago
|
||
This is the same as attachment 198443 [details] [diff] [review] (from bug 310589).
Attachment #305065 -
Flags: superreview?(roc)
Attachment #305065 -
Flags: review?(roc)
Assignee | ||
Updated•15 years ago
|
Attachment #305065 -
Attachment description: add nuull-check → add null-check
Assignee | ||
Comment 20•15 years ago
|
||
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+
Assignee | ||
Comment 22•15 years ago
|
||
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.
Assignee | ||
Comment 23•15 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
That plus an assertion would be good.
Assignee | ||
Comment 25•15 years ago
|
||
Checked in a followup with comment+assertion+initialization of result.mOffset.
Reporter | ||
Comment 26•15 years ago
|
||
Verified fixed with the attached testcases.
Status: RESOLVED → VERIFIED
Updated•11 years ago
|
Crash Signature: [@ FindBlockFrameOrBR]
Comment 27•10 years ago
|
||
dveditz, I think we can open this bug now. I don't see any sensitive information here, including all testcases.
Updated•9 years ago
|
Group: core-security
Comment 28•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce53ea4d732d
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•