Closed Bug 319705 Opened 19 years ago Closed 19 years ago

Crash when deleting comment line selected with triple click [@nsFontMetricsWin::ResolveForwards]

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Platform:
x86
Windows 98
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: hhschwab, Assigned: uriber)

References

Details

(Keywords: regression)

Attachments

(2 files)

minimal testcase
52 bytes, text/html
Details
patch
801 bytes, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051208 SeaMonkey/1.5a
TB12779146Q, TB12778107Y

also crashing in branch: 1.8: 2005120309

regressed between seamonkey 1.9a1: 2005091805 and 2005091906
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-09-18+04%3A00&maxdate=2005-09-19+06%3A00&cvsroot=%2Fcvsroot

Bug 306895
Triple click should select lines, not paragraphs, in "white-space: -moz-pre-wrap;"

I'll attach a testcase containing a three-line-comment:
<html><head>
<!--
comment line 1
-->
</head><body><br>
</body></html>

Steps to repeat:
1. load testcase in composer (or browser and press CTRL+E)
2. select first or second line of comment using triple-click
3. delete line -> crash

no crash, if line is selected not using triple-click.

TB12778107Y:

nsFontMetricsWin::ResolveForwards  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/gfx/src/windows/nsFontMetricsWin.cpp, line 4028]
nsRenderingContextWin::GetWidth  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/gfx/src/windows/nsRenderingContextWin.cpp, line 1522]
nsTextFrame::GetPointFromOffset  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsTextFrame.cpp, line 4377]
nsTypedSelection::GetPointFromOffset  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsSelection.cpp, line 6705]
nsTypedSelection::GetCachedFrameOffset  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsSelection.cpp, line 5048]
nsCaret::GetCaretRectAndInvert  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCaret.cpp, line 1054]
nsCaret::DrawAtPositionWithHint  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCaret.cpp, line 585]
nsCaret::DrawCaret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCaret.cpp, line 974]
nsCaret::StartBlinking  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCaret.cpp, line 525]
PresShellViewEventListener::DidRefreshRegion  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp, line 7366]
nsViewManager::DispatchEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp, line 2047]
HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp, line 176]
nsWindow::DispatchEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 1162]
nsWindow::ProcessMessage  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 4284]
nsWindow::WindowProc  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 1351]
KERNEL32.DLL + 0x363b (0xbff7363b)
KERNEL32.DLL + 0x242e7 (0xbff942e7)
Attached file minimal testcase — 
<html>
<!--
comment line 1
-->        
</html>

Steps to repeat:

1. load testcase in composer (or browser and press CTRL+E)
2. at the bottom, select tab <HTML> Source
3. using triple-click, select 1st or 2nd line of comment.
4. delete line -> crash

no crash, if last line of comment is selected, or selection was done not using triple-click.
select first or 2nd line of comment using triple-click
Taking. I'll look at this next week.
Assignee: composer → uriber
I get:

###!!! ASSERTION: invalid offset passed to GetPointFromOffset: '0', file ../../../mozilla/layout/generic/nsTextFrame.cpp, line 4350
###!!! ASSERTION: invalid offset passed to GetPointFromOffset: '0', file ../../../mozilla/layout/generic/nsTextFrame.cpp, line 4350
Blocks: 306895
Component: Composer → Layout: BiDi Hebrew & Arabic
No longer depends on: 306895
Product: Mozilla Application Suite → Core
Given those asserts, we're probably looking at random data on the heap, so this might be exploitable.

The one thing that confuses me is that Hermann says this crashes on branch but bug 306895 was never checked in on branch, right?  If so, what's actually going on on branch?
Component: Layout: BiDi Hebrew & Arabic → Layout: Block and Inline
(In reply to comment #4)
> what's actually going on on branch?

For some time, the branch builds can only be downloaded from contribute folders  and don't have talkback packed.

I did download some more, and found them all crashing, seems there got a bug fixed only in the 1.9 trunk, later regressing into this bug.


Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050816 SeaMonkey/1.0a
seems to give same talkback as current builds, but offsets only into libraries.

Incident ID: 12799807
Stack Signature	GKGFXWIN.DLL + 0xa3b5 (0x6022a3b5) 04d17ccc
Product ID	MozillaTrunk
Build ID	2005081612

Seamonkey 1.8 tested crashing, but no talkback available with branch builds:

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050816 SeaMonkey/1.0a
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050910 SeaMonkey/1.0a
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050916 SeaMonkey/1.0a
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050918 SeaMonkey/1.0a
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8)   Gecko/20051208 SeaMonkey/1.0b
I couldn't actually reproduce the reported crash (it's in Windows-specific code, and I'm on mac), but I did manage to crash elesewhere following similar steps as in the report, and I did get the asserts that bz quoted. So I concentrated on investigating the asserts.

Here's what I found:
Deleting the triple-click-selected text apparently leaves a text frame consisting of a single CR (0x0d) character. This might be a bad thing by itself (I'll try looking into it next), but I think that the real problem is that GetPointFromOffset() and PrepareUnicodeText() don't handle such a frame well.
What happens is that PrepareUnicodeText() in this case leaves indexBuffer.mBuffer uninitialized (the single character in the content is not mapped to anything). Later, when GetPointFromOffset() examines ip[inOffset] (which is really indexBuffer.mBuffer[0]), it finds junk there, and from there on all kinds of things can happen.
So I suspect the problem is in PrepareUnicodeText(). However, I don't quite understand this code, and I can't even say what aIndexBuffer.mBuffer[0] should be set to in this case. If anyone can provide some extra insight for me that would be great.
Some more detail:

The reason PrepareUnicodeText() does not init indexBuffer.mBuffer[0] is that the call to aTX.GetNextWord() returns nsnull, which then causes an immediate break out of the loop.
I can't say whether GetNextWord() really should return nsnull in this case (and then it should just be handled better by PrepareUnicodeText()), or whether the problem is in GetNextWord() itself.
I can crash (due to the same underlying problem) by moving the caret over a blank comment line when the HTML source uses DOS linebreaks (CRLF) - without using triple-click at all. This might explain crashes on the branch (which does not have the patch for bug 306895).

It seems that when building the frames for displaying HTML source, the CRs are stripped together with the LFs in "regular" HTML, but are left intact (while LFs are stripped) in HTML comments. I'm not sure whether this is intentional or not. Changing this behavior would likely fix these crashes, although fixing PrepareUnicodeText()/GetNextWord() would probably be a better solution.
Please file a bug on parser about LF in comments -- it should be removed, I believe.

That said, since DOM scripting can introduce LF into the DOM (creating invalid documents, but that's the DOM for you), we shouldn't be crashing on hitting LF.
(In reply to comment #9)
> Please file a bug on parser about LF in comments -- it should be removed, I
> believe.

I took a closer look at this. During parsing, it seems that all CRs and LFs in comments are simply left intact (I verified this by doing a content dump). Do you consider this a problem?
When creating the "Source" view in composer, LFs are replaced by BRFrames, while CRs are left intact. So that a CRLF sequence turns into a CR character (as part of a TextFrame) followed by a BRFrame. I suspect the problem is actually at this stage, not in parsing of the original HTML.

If you still want me to file a parser bug, what would be a good summary? "line breaks in comments should be standardized"? "CRs in comments should be removed"?
Attached patch patch — — Splinter Review 
This patch fixes the specific scenario described in comment #0 (i.e., the regression due to bug 306895). There is no need to check for '\r' (CR) at ends of strings, as all types of newlines are apparently normalized to '\n' (LF) in white-space:-moz-pre-wrap.
This will cause the CR in the comment to be selected (and then deleted) with the rest of the text, and therefore we won't end up with a CR-only line.

I suggest taking this patch anyway, but then also trying to fix the root cause (i.e. the crash on CR-only lines).
Attachment #206790 - Flags: review?(bzbarsky)
> it seems that all CRs and LFs in comments are simply left intact (I verified
> this by doing a content dump). Do you consider this a problem?

Yes.  The DOM newline character is LF.  All CRs should be removed or converted to LFs by the parser.

> "CRs in comments should be removed"?

That sounds reasonable.  Please cc me and mbrkap, ok?
Comment on attachment 206790 [details] [diff] [review]
patch

Yeah, indeed.  \r is not a newline in the DOM.
Attachment #206790 - Flags: superreview+
Attachment #206790 - Flags: review?(bzbarsky)
Attachment #206790 - Flags: review+
Patch checked in.
I'm marking this bug fixed.
Hermann (or someone else), please verify on Windows with the next nightly.

I'll file a separate bug about the parsing issue, and (if I can create a testcase), also a bug about the root cause of the crash (CR-only line).
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
(In reply to comment #12)
> The DOM newline character is LF.  All CRs should be removed or converted
> to LFs by the parser.

Submitted bug 321484 on this.
(In reply to comment #9)
> That said, since DOM scripting can introduce LF into the DOM (creating invalid
> documents, but that's the DOM for you), we shouldn't be crashing on hitting LF.

I filed bug 321487 for this issue.

wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051225 SeaMonkey/1.5a

crashed with the 2005122409 nightly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: