Bug 3739 - Unreproducible - Assertion failure in isEqualIgnoringAffinity on double-click
: Unreproducible - Assertion failure in isEqualIgnoringAffinity on double-click
Status: VERIFIED FIXED
: WebKit
Layout and Rendering
: 412
: Macintosh Mac OS X 10.4
: P2 Major
Assigned To:
: http://bugzilla.opendarwin.org/show_b...
:
:
:
  Show dependency treegraph
 
Reported: 2005-06-27 12:44 PST by
Modified: 2005-11-08 13:17 PST (History)


Attachments
Newer crash log (21.09 KB, text/plain)
2005-08-30 09:35 PST, Alexey Proskuryakov
no flags Details
Force DOWNSTREAM at end of paragraph (1.21 KB, patch)
2005-10-04 08:34 PST, Alexey Proskuryakov
harrison: review-
Review Patch | Details | Formatted Diff | Diff
Automated test (2.85 KB, patch)
2005-10-04 09:11 PST, Alexey Proskuryakov
no flags Review Patch | Details | Formatted Diff | Diff
RenderText::inlineBox fix (1.46 KB, patch)
2005-11-04 10:57 PST, David Harrison
no flags Review Patch | Details | Formatted Diff | Diff
atLineWrap (4.75 KB, patch)
2005-11-07 16:50 PST, David Harrison
harrison: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2005-06-27 12:44:20 PST
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
-------------------------------------------------------------------
------- Comment #1 From 2005-06-27 23:32:31 PST -------
Can't reproduce it, will leave it open for anyone else to try, please provide further insight in how to 
recreate it.
------- Comment #2 From 2005-06-28 10:40:01 PST -------
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.
------- Comment #3 From 2005-06-30 11:39:56 PST -------
Confirming it, cause a crash happened, but i can't reproduce.
------- Comment #4 From 2005-06-30 11:44:40 PST -------
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.
------- Comment #5 From 2005-06-30 15:04:27 PST -------
Same as <rdar://problem/4140688> assertion failure double-clicking text to select.

The fix for that has been submitted.  Moving this bug to FIXED.
------- Comment #6 From 2005-07-02 03:42:02 PST -------
The fix seems to be not in cvs, so cannot verify yet.
------- Comment #7 From 2005-08-29 16:21:54 PST -------
The fix is in WebCore/khtml/rendering/render_text.cpp, revision 183, which was committed on 27 June 
2005.
------- Comment #8 From 2005-08-30 09:34:00 PST -------
It looks like a different issue then - I can still reproduce it with ToT (clean rebuild, render_text.cpp at 
revision 1.192). Reopening.
------- Comment #9 From 2005-08-30 09:35:24 PST -------
Created an attachment (id=3674) [details]
Newer crash log

Line numbers have changed a lot, so here is a current crash log.
------- Comment #10 From 2005-09-01 11:05:59 PST -------
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.
------- Comment #11 From 2005-09-01 11:50:41 PST -------
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).
------- Comment #12 From 2005-10-04 08:34:32 PST -------
Created an attachment (id=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
------- Comment #13 From 2005-10-04 09:11:09 PST -------
Created an attachment (id=4192) [details]
Automated test
------- Comment #14 From 2005-10-04 12:39:50 PST -------
(From update of attachment 4191 [details])
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.
------- Comment #15 From 2005-10-04 12:42:14 PST -------
(there I go again saying "line" when I mean "paragraph"...)
------- Comment #16 From 2005-11-04 10:57:10 PST -------
Created an attachment (id=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.
------- Comment #17 From 2005-11-04 13:02:03 PST -------
Committed.
------- Comment #18 From 2005-11-04 13:07:49 PST -------
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.
------- Comment #19 From 2005-11-04 22:15:26 PST -------
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.)
------- Comment #20 From 2005-11-07 06:19:40 PST -------
(From update of attachment 4597 [details])
Removing review+
------- Comment #21 From 2005-11-07 16:50:25 PST -------
Created an attachment (id=4625) [details]
atLineWrap

Fixes caretRect in similar manner as inlineBox was.  In fact, the logic is now
shared in new function RenderText::atLineWrap.
------- Comment #22 From 2005-11-07 16:52:03 PST -------
(From update of attachment 4625 [details])
Justin and Hyatt approved this.   I added the automated test as
editing/selection/doubleclick-crash.html.
------- Comment #23 From 2005-11-07 16:52:56 PST -------
Committed,
------- Comment #24 From 2005-11-08 13:17:56 PST -------
Verified, the assertion no longer fires!