RESOLVED FIXED 66120
Incorrect data retrieved in calls to get selection data in client->textWillBeReplaced() from CharacterData::setDataAndUpdate 
https://bugs.webkit.org/show_bug.cgi?id=66120
Summary Incorrect data retrieved in calls to get selection data in client->textWillBe...
Jon Lee
Reported 2011-08-11 17:58:46 PDT
The call to textWillBeReplaced() in CharacterData::setDataAndUpdate() is performed before the text node gets updated, and before the document is notified that text has been removed. textWillBeReplaced() updates the selection, which in turn notifies the client. If the client decides to retrieve the selection, or create a Range based on the data or selection, the client would retrieve incorrect data for the following reasons: 1) Since the DOM node has not been updated yet, the desired selection/caret might be outside of the available DOM range. (For example, if the user types a character) 2) Any ranges created by the client into the document would get adjusted by the textRemoved() call. It would seem to make sense to move the textWillBeReplaced() call to after the textRemoved() call, but I would appreciate some input as to why this call is made when it is made.
Attachments
Patch (4.28 KB, patch)
2012-06-06 00:55 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch for landing (4.34 KB, patch)
2012-06-06 23:31 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Jon Lee
Comment 1 2011-08-11 17:59:14 PDT
<rdar://problem/9922944>
Ryosuke Niwa
Comment 2 2011-08-11 18:11:57 PDT
(In reply to comment #0) > It would seem to make sense to move the textWillBeReplaced() call to after the textRemoved() call, but I would appreciate some input as to why this call is made when it is made. If we do this, then you'd have to update the code in textWillBeReplaced so that it'll update selection accordingly. The current code relies on the fact, it's called before the node is updated.
Shinya Kawanaka
Comment 3 2012-06-06 00:55:50 PDT
Created attachment 145955 [details] Patch
Ryosuke Niwa
Comment 4 2012-06-06 00:58:34 PDT
Comment on attachment 145955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145955&action=review Did you run all the layout tests with this change? I'd expect hundreds of tests to fail. > Source/WebCore/ChangeLog:12 > + No new tests, refactoring. This is definitely not true. This change WILL change the webkit's behavior. A better justification will be that we can't test this in layout tests.
Shinya Kawanaka
Comment 5 2012-06-06 02:29:59 PDT
(In reply to comment #4) > (From update of attachment 145955 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145955&action=review > > Did you run all the layout tests with this change? I'd expect hundreds of tests to fail. As far as I tried, only around 10 tests failed. I'm not sure it's regression or not though... I have to run tests in ToT again... > > > Source/WebCore/ChangeLog:12 > > + No new tests, refactoring. > > This is definitely not true. This change WILL change the webkit's behavior. > A better justification will be that we can't test this in layout tests. OK.
Shinya Kawanaka
Comment 6 2012-06-06 02:30:59 PDT
> As far as I tried, only around 10 tests failed. I'm not sure it's regression or not though... > I have to run tests in ToT again... Actually I hoped EWS to test them... But chromium bot is down now. Too bad.
Ryosuke Niwa
Comment 7 2012-06-06 12:23:56 PDT
Comment on attachment 145955 [details] Patch r=me as long as tests pass. Please update the change log as advised.
Shinya Kawanaka
Comment 8 2012-06-06 23:31:30 PDT
Created attachment 146207 [details] Patch for landing
WebKit Review Bot
Comment 9 2012-06-07 04:45:36 PDT
Comment on attachment 146207 [details] Patch for landing Clearing flags on attachment: 146207 Committed r119705: <http://trac.webkit.org/changeset/119705>
WebKit Review Bot
Comment 10 2012-06-07 04:45:41 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 11 2012-08-16 00:02:31 PDT
Re-opened since this is blocked by 94192
Abhishek Arya
Comment 12 2012-08-16 00:07:37 PDT
*** Bug 91325 has been marked as a duplicate of this bug. ***
Abhishek Arya
Comment 13 2012-08-16 00:08:43 PDT
(In reply to comment #12) > *** Bug 91325 has been marked as a duplicate of this bug. *** Please do see the testcase in 91325 which reproduces 100% reliably on asanified version of chromium linux and explanation of the problem in c#13.
Ryosuke Niwa
Comment 14 2012-08-16 01:45:10 PDT
(In reply to comment #13) > (In reply to comment #12) > > *** Bug 91325 has been marked as a duplicate of this bug. *** > > Please do see the testcase in 91325 which reproduces 100% reliably on asanified version of chromium linux and explanation of the problem in c#13. This is crazy. Why was the patch rolled out without any prior notice or a formal review.
Abhishek Arya
Comment 15 2012-08-16 07:06:23 PDT
http://trac.webkit.org/changeset/125760
Note You need to log in before you can comment on or make changes to this bug.