RESOLVED FIXED Bug 39095
REGRESSION (r53085): Crash when changing writing direction to right-to-left 
https://bugs.webkit.org/show_bug.cgi?id=39095
Summary REGRESSION (r53085): Crash when changing writing direction to right-to-left
Xiaomei Ji
Reported 2010-05-13 17:59:41 PDT
Created attachment 56037 [details] core dump file Open the attached test case, following instructions in the page. Safari crashes when changing the writing directions to right-to-left. Stack overflow. Attached the core dump. Might be caused by http://trac.webkit.org/changeset/53085/trunk/WebCore/dom/Position.cpp
Attachments
core dump file (123.20 KB, text/plain)
2010-05-13 17:59 PDT, Xiaomei Ji 		no flags
Details
test case (674 bytes, text/html)
2010-05-13 18:00 PDT, Xiaomei Ji 		no flags
Details
patch (3.88 KB, patch)
2010-05-14 19:58 PDT, Adele Peterson 		hyatt: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Xiaomei Ji
Comment 1 2010-05-13 18:00:23 PDT
Created attachment 56038 [details] test case
Xiaomei Ji
Comment 2 2010-05-13 18:03:10 PDT
Chromium bug: http://crbug.com/43734
Yair Yogev
Comment 3 2010-05-14 00:02:17 PDT
note: if the div was RTLed by default, doing the same steps to change the direction to LTR would have the same results
Mark Rowe (bdash)
Comment 4 2010-05-14 04:13:04 PDT
<rdar://problem/7984158>
Xiaomei Ji
Comment 5 2010-05-14 17:55:47 PDT
I looked at the code a bit. Since I do not have much knowledge of the background, I do not have much idea on in which condition the recursive call should exit. The original intention was to focus on empty "span" inside non-editable "div". But the code seems does it in more broader scope. I am wondering 1. why focus on empty inline element in the first place? currently, moving cursor within that element cause coredump too. Refer bug 38923. 2. in which element/inline-element pair should the focus be allowed? Thanks, xiaomei
Adele Peterson
Comment 6 2010-05-14 18:42:03 PDT
I'm looking into this as well. I'm not an expert here, but I'm trying to understand what's going on.
Adele Peterson
Comment 7 2010-05-14 18:56:33 PDT
From looking at the history here, it seems that the case in Position::getInlineBoxAndOffset that deals with inlines (line 1033) was added before the rest of the method crossed editable boundaries correctly. So I think when Dan added the use of downstreamIgnoringEditingBoundaries and upstreamIgnoringEditingBoundaries earlier in the method, then we no longer need that case. So I'm testing a fix that just removes that, and so far so good. It fixes the test case.
Adele Peterson
Comment 8 2010-05-14 19:58:08 PDT
Created attachment 56134 [details] patch I talked about this briefly with Enrica, and she thought it made sense. I'm not 100% confident, but it seems ok.
Adele Peterson
Comment 9 2010-05-14 19:58:49 PDT
also, the name of my test is not great. suggestions welcome!
Dave Hyatt
Comment 10 2010-05-14 21:04:52 PDT
Comment on attachment 56134 [details] patch r=me
Adele Peterson
Comment 11 2010-05-14 21:46:24 PDT
Committed revision 59516.
Xiaomei Ji
Comment 12 2010-05-17 09:24:59 PDT
Thanks for the fix, especially during the weekend. Appreciated! Also I learned how to create JS test case (from where to look for those execCommand).
Note You need to log in before you can comment on or make changes to this bug.