Bug 3739 – Unreproducible - Assertion failure in isEqualIgnoringAffinity on double-click

• Status: VERIFIED
• Resolution: FIXED
• Component: Layout and Rendering

[Alexey Proskuryakov]

In development build of ToT WebKit, an assertion often fires after a double-click in an empty area.

Steps to reproduce:
1. Go to http://bugzilla.opendarwin.org/show_bug.cgi?id=3510 (somehow, I can most reliably 
reproduce this issue on that page ;-) ) 
2. Scroll down to the beginning of the description
3. Double-click after the end of the first paragraph (after "Accept-Language.")

Results: with a high probability, an assertion fires (causing a crash in development build).

Discussion: I have dumped the values causing the assertion to fire:

-------------------------------------------------------------------
 a.m_deepPosition: "offset 129 of #text; value=If the system primary language (as set in International 
control panel) is Russian, then only "ru-ru" is 
sent in Accept-Language.

Steps to reproduce:
1. Set the system primary language to Russian (on my machine, the exact order is Russian, English, 
Japanese, Chinese Traditional, Chinese Simplified).
2. In Safari, go to "
 b.m_deepPosition: "offset 129 of #text; value=If the system primary language (as set in International 
control panel) is Russian, then only "ru-ru" is 
sent in Accept-Language.

Steps to reproduce:
1. Set the system primary language to Russian (on my machine, the exact order is Russian, English, 
Japanese, Chinese Traditional, Chinese Simplified).
2. In Safari, go to "
 a.m_affinity: 0
 b.m_affinity: 1
-------------------------------------------------------------------

Here is the crash log:
-------------------------------------------------------------------
0   com.apple.WebCore        	0x01883200 khtml::isEqualIgnoringAffinity(khtml::VisiblePosition 
const&, khtml::VisiblePosition const&) + 352 (visible_position.cpp:396)
1   com.apple.WebCore        	0x0188b084 khtml::isEndOfParagraph(khtml::VisiblePosition const&) + 
92 (visible_units.cpp:663)
2   com.apple.WebCore        	0x0187e30c khtml::Selection::validate(khtml::ETextGranularity) + 1644 
(selection.cpp:863)
3   com.apple.WebCore        	0x0187f290 khtml::Selection::expandUsingGranularity
(khtml::ETextGranularity) + 72 (selection.cpp:499)
4   com.apple.WebCore        	0x0165c4d8 KHTMLPart::selectClosestWordFromMouseEvent
(QMouseEvent*, DOM::NodeImpl*, int, int) + 260 (khtml_part.cpp:4582)
5   com.apple.WebCore        	0x0165c630 KHTMLPart::handleMousePressEventDoubleClick
(khtml::MousePressEvent*) + 216 (khtml_part.cpp:4608)
6   com.apple.WebCore        	0x0165fc7c KHTMLPart::khtmlMousePressEvent
(khtml::MousePressEvent*) + 616 (khtml_part.cpp:4721)
7   com.apple.WebCore        	0x015f1fe4 KWQKHTMLPart::khtmlMousePressEvent
(khtml::MousePressEvent*) + 460 (KWQKHTMLPart.mm:2012)
8   com.apple.WebCore        	0x0165b60c KHTMLPart::customEvent(QEvent*) + 88 (khtml_part.cpp:
4508)
9   com.apple.WebCore        	0x0190284c KParts::Part::event(QEvent*) + 64 (KWQKPartsPart.h:56)
10  com.apple.WebCore        	0x01911a40 QApplication::sendEvent(QObject*, QEvent*) + 64 
(KWQApplication.h:38)
11  com.apple.WebCore        	0x0166cd80 KHTMLView::viewportMousePressEvent(QMouseEvent*) + 
736 (KWQPtrList.h:807)
12  com.apple.WebCore        	0x015f0b74 KWQKHTMLPart::mouseDown(NSEvent*) + 532 
(KWQKHTMLPart.mm:2611)
13  com.apple.WebCore        	0x0164e7f4 -[WebCoreBridge mouseDown:] + 52 (WebCoreBridge.mm:
864)
14  com.apple.WebKit         	0x0037d694 -[WebHTMLView mouseDown:] + 356 (WebHTMLView.m:
2683)
15  com.apple.AppKit         	0x93686d28 -[NSWindow sendEvent:] + 4616
16  com.apple.Safari         	0x0001d6bc 0x1000 + 116412
17  com.apple.AppKit         	0x9362ff5c -[NSApplication sendEvent:] + 4172
18  com.apple.Safari         	0x0001a6a4 0x1000 + 104100
19  com.apple.AppKit         	0x936273f0 -[NSApplication run] + 508
20  com.apple.AppKit         	0x93717c1c NSApplicationMain + 452
21  com.apple.Safari         	0x00002700 0x1000 + 5888
22  com.apple.Safari         	0x00057190 0x1000 + 352656
-------------------------------------------------------------------

[Joost de Valk (AlthA)]

Can't reproduce it, will leave it open for anyone else to try, please provide further insight in how to 
recreate it.

[Alexey Proskuryakov]

For me, crashing usually occurs when double-clicking in the whitespace after a paragraph end if that 
paragraph uses a non-proportional font. Besides Bugzilla, I've now reproduced it with messages in 
lists.apple.com/archives.

I have tried changing the font (Monaco or Courier at different sizes), but the results were the same.

I have mentioned it, but didn't stress - AFAIK, one needs to run a development build of WebKit (set-
webkit-configuration --development) to see assertions.

Even if this issue stays unreproducible, perhaps the crash log combined with the data could lead someone 
to discovering its cause? There are some hints in isEqualIgnoringAffinity() code.

[Joost de Valk (AlthA)]

Confirming it, cause a crash happened, but i can't reproduce.

[John Sullivan]

There's another report of this in radar bug <rdar://problem/4054701>. That case involves using 
VoiceOver and Mail, with these steps:

1. Using TOT build of Safari, launch Mail from terminal. Type three words in the compose window.
2. command F5 to launch VoiceOver
3. Bring VoiceOver cursor into the compose portion of the window.  Ctrl-Option-end, then ctrl-option-
shift-down arrow.
4. Read word by word using ctrl-option-right arrow until you reach the last word.
5. Upon reaching the last word, WebCore crashes.

Dave Harrison confirms in 4054701 that there are no ill effects from this assert on deployment builds, 
but it does indicate an unexpected condition, and should be addressed. Assigning this to him.

[David Harrison]

Same as <rdar://problem/4140688> assertion failure double-clicking text to select.

The fix for that has been submitted.  Moving this bug to FIXED.

[Alexey Proskuryakov]

The fix seems to be not in cvs, so cannot verify yet.

[David Harrison]

The fix is in WebCore/khtml/rendering/render_text.cpp, revision 183, which was committed on 27 June 
2005.

[Alexey Proskuryakov]

It looks like a different issue then - I can still reproduce it with ToT (clean rebuild, render_text.cpp at 
revision 1.192). Reopening.

[Alexey Proskuryakov]

Created attachment 3674 [details]
Newer crash log

Line numbers have changed a lot, so here is a current crash log.

[David Harrison]

This should be fixed by the changes for

<rdar://problem/4054701> "assertion failure in khtml::isEqualIgnoringAffinity using VoiceOver in new 
Mail message"

I've just committed that fix.

[Alexey Proskuryakov]

Unfortunately, it still crashes for me (render_text.cpp at version 1.195, clean rebuild). The stack trace 
hasn't changed (except for line numbers, of course).

[Alexey Proskuryakov]

Created attachment 4191 [details]
Force DOWNSTREAM at end of paragraph

This is almost a shot in the dark, but:
1) it fixes the (100% reproducible) assertion failure for me;
2) and it really implements what is suggested by a comment in
visible_position.h:
    // NOTE: UPSTREAM affinity will be used only if pos is at end of a wrapped
line,
    // otherwise it will be converted to DOWNSTREAM

[Alexey Proskuryakov]

Created attachment 4192 [details]
Automated test

[David Harrison]

Comment on attachment 4191 [details]
Force DOWNSTREAM at end of paragraph

This patch does not address the root cause of the problem, which is that we
need to define how affinity applies to newline characters in text nodes with
whitespace:PRE style.	The assert fires because of an inconsistency:
RenderText::inlineBox() acts as if affinity applies to newlines in preformatted
text, but endOfParagraph() does not (it always produces DOWNSTREAM affinity).

The better fix is to decide a policy and make it consistent.  My gut says to
have affinity NOT apply in this case, because affinity is a "line wrap" concept
not a paragraph break concept (even if that break is within a text node, it is
not the same as "line wrap").  This would mean modifying
RenderText::inlineBox() to heed newlines in whitespace:PRE.  It probably means
other changes, as well, because this nuance was AFAIK not considererd when
writing the affinity-related code.  For example,  I locally fixed
RenderText::inlineBox(), and single-clicking at the end of the line leaves the
cursor blinking at the beginning of the second line.

[David Harrison]

(there I go again saying "line" when I mean "paragraph"...)

[David Harrison]

Created attachment 4597 [details]
RenderText::inlineBox fix

This patch addresses the root cause of the problem by making
RenderText::inlineBox() cope with the fact that in white-space:pre text the
newline characters are not part of any InlineTextBox... they lie "between"
them.	Now DOWNSTREAM affinity selects the next text box only if the offset
that is past the current box is actually _in_ the next box.

[David Harrison]

Committed.

[David Harrison]

Now I have to look at the follow-on problem: single-clicking at the end of the line leaves the
cursor blinking at the beginning of the next line.

[Geoffrey Garen]

Dave, do you want to close this bug, and open a new bug for the follow-on you mentioned? (This bug 
misleadingly shows up in the "to be landed" queue.)

[David Harrison]

Comment on attachment 4597 [details]
RenderText::inlineBox fix

Removing review+

[David Harrison]

Created attachment 4625 [details]
atLineWrap

Fixes caretRect in similar manner as inlineBox was.  In fact, the logic is now
shared in new function RenderText::atLineWrap.

[David Harrison]

Comment on attachment 4625 [details]
atLineWrap

Justin and Hyatt approved this.   I added the automated test as
editing/selection/doubleclick-crash.html.

[David Harrison]

Committed,

[Alexey Proskuryakov]

Verified, the assertion no longer fires!