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

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

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

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Layout and Rendering (show other bugs)
Version:	528+ (Nightly build)
Hardware:	All All

Importance:	P2 Normal
Assignee:	Nobody

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2010-05-13 17:59 PDT by Xiaomei Ji
Modified:	2010-05-17 09:24 PDT (History)
CC List:	5 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

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

Description Xiaomei Ji 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

Comment 1 Xiaomei Ji 2010-05-13 18:00:23 PDT

Created attachment 56038 [details]
test case

Comment 2 Xiaomei Ji 2010-05-13 18:03:10 PDT

Chromium bug:
http://crbug.com/43734

Comment 3 Yair Yogev 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

Comment 4 Mark Rowe (bdash) 2010-05-14 04:13:04 PDT

<rdar://problem/7984158>

Comment 5 Xiaomei Ji 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

Comment 6 Adele Peterson 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.

Comment 7 Adele Peterson 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.

Comment 8 Adele Peterson 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.

Comment 9 Adele Peterson 2010-05-14 19:58:49 PDT

also, the name of my test is not great.  suggestions welcome!

Comment 10 Dave Hyatt 2010-05-14 21:04:52 PDT

Comment on attachment 56134 [details]
patch

r=me

Comment 11 Adele Peterson 2010-05-14 21:46:24 PDT

Committed revision 59516.

Comment 12 Xiaomei Ji 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).