Bug 39095

Summary:

REGRESSION (r53085): Crash when changing writing direction to right-to-left

Product:

WebKit

Reporter:

Xiaomei Ji <xji>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

adele, enrica, mitz, progame+wk, xji

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
core dump file	none
test case	none
patch	hyatt: review+

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+	Details Formatted Diff Diff
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.