Summary: | Unreproducible - Assertion failure in isEqualIgnoringAffinity on double-click | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||||||
Component: | Layout and Rendering | Assignee: | David Harrison <harrison> | ||||||||||||
Status: | VERIFIED FIXED | ||||||||||||||
Severity: | Major | ||||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 412 | ||||||||||||||
Hardware: | Mac | ||||||||||||||
OS: | OS X 10.4 | ||||||||||||||
URL: | http://bugzilla.opendarwin.org/show_bug.cgi?id=3510 | ||||||||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2005-06-27 12:44:20 PDT
Can't reproduce it, will leave it open for anyone else to try, please provide further insight in how to recreate it. 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. Confirming it, cause a crash happened, but i can't reproduce. 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. Same as <rdar://problem/4140688> assertion failure double-clicking text to select. The fix for that has been submitted. Moving this bug to FIXED. The fix seems to be not in cvs, so cannot verify yet. The fix is in WebCore/khtml/rendering/render_text.cpp, revision 183, which was committed on 27 June 2005. It looks like a different issue then - I can still reproduce it with ToT (clean rebuild, render_text.cpp at revision 1.192). Reopening. Created attachment 3674 [details]
Newer crash log
Line numbers have changed a lot, so here is a current crash log.
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. 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). 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
Created attachment 4192 [details]
Automated test
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.
(there I go again saying "line" when I mean "paragraph"...) 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.
Committed. 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. 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 on attachment 4597 [details]
RenderText::inlineBox fix
Removing review+
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.
Comment on attachment 4625 [details]
atLineWrap
Justin and Hyatt approved this. I added the automated test as
editing/selection/doubleclick-crash.html.
Committed, Verified, the assertion no longer fires! |