REGRESSION(r8780): Backwards delete by word incorrectly appends deleted text to kill ring, should be prepend

• Component: HTML Editing
• Status: RESOLVED FIXED

[BJ Burg]

SUMMARY:

Backward text deletions should prepend the deleted text to the kill ring, but instead it appends. Yanking the kill sequence results in the wrong yank text since it was added in the wrong order.

STEPS TO REPRODUCE:

 * Add the text '123 456' to a contenteditable or form element.
 * Move caret to | position: '123 456|'
 * Backwards delete-by-word twice (on Mac Safari, Option-Delete)
 * Yank (on Mac Safari, Ctrl-Y)

EXPECTED:

* yanked text should be the original text, '123 456'

ACTUAL:

* yanked text is actually '456123 '

NOTES:

This seems to be a really really old bug, which was discovered while removing an apparently unused argument in https://bugs.webkit.org/show_bug.cgi?id=151157.
When deleting backwards / leftwards, the killed text should be prepended instead of appended to the kill ring.

[BJ Burg]

I am not even kidding about the title change, Darin regressed this almost 11 years ago:

http://trac.webkit.org/changeset/8780


The relevant mistake can be seen here, where the code no longer computes 'prepend' based on the selection direction, nor does it change to disambiguate inside _deleteRange using the passed-in deletion type enum.

https://github.com/WebKit/webkit/commit/05b622f#diff-2929972b50734b9742ad9676920551a0L4153

Sorry Darin! This should not be too difficult to fix, though the code has moved about 30 times in 11 years. Might take a while to find the exact place to do it.

[BJ Burg]

For posterity, this is how I was able to find the regression:

-----

How I figured out the addToKillRing regression:

* unused parameter 'append', where did it come from?
* Initially searched commit logs for addToKillRing, too much unrelated refactoring.

* Used `git blame`, too many unrelated refactorings. Went to Google to find something that can search changes to a specific line rather than just the last line.

* found out about git pickaxe `git log -S "text"` which searches for increment or decrement to count of "text" occurences in a file because of a diff. This led me to a commit where addToKillRing was moved to a different directory in a massive reorg.

$ git log -p --oneline -S "addToKillRing" Source/WebCore/editing/ | grep -C5 addToKillRing

* found out about git pickaxe variant `git log -G "regex" which searches for any match to regex in diffs. This would let me find changes to the line that both include addToKillRing.

$ git log -p --oneline -G "addToKillRing\\(" WebCore | grep -C5 --null addToKillRing

* Looked for earliest result to see if it was regressed there. It was not.

* Earliest result is another refactor that changed call signature and name (from ObjC to C++). I continued searching for the ObjC function name.

$ git log -p --oneline -G "_deleteRange:" -- WebKit

* Narrow down just to the relevant arguments

$ git log --oneline -G "killRing:YES prepend:" -- WebKit

* Manually check the newest and oldest revision. The oldest (initial implementation) was correct.

* Start checking commits one-by-one from oldest to newest, found the regression after a few revisions. The trac revision is listed in the commit message after 'git-svn-id'.

[Radar WebKit Bug Importer]

<rdar://problem/23558868>

[Darin Adler]

Really glad you found this! Shows how important it is to have test cases.

[BJ Burg]

Created attachment 265856 [details]
Proposed Fix

[Darin Adler]

Comment on attachment 265856 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=265856&action=review

> Source/WebCore/editing/Editor.h:340
> +    void addRangeToKillRing(Range*, KillRingInsertionMode);

Should take a Range& instead of a Range* since it can never be null.

> Source/WebCore/platform/mac/KillRingMac.mm:52
> -    static bool initializedKillRing = false;
> -    if (!initializedKillRing) {
> -        initializedKillRing = true;
> +    static std::once_flag onceFlag;
> +    std::call_once(onceFlag, [] {
>          _NSInitializeKillRing();
> -    }
> +    });

Why this change? The code is bigger and more complex. Do we need to support calling this function on threads other than the main thread? I don’t think we do.

[BJ Burg]

Comment on attachment 265856 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=265856&action=review

>> Source/WebCore/platform/mac/KillRingMac.mm:52
>> +    });
> 
> Why this change? The code is bigger and more complex. Do we need to support calling this function on threads other than the main thread? I don’t think we do.

Ah, I forgot that the std lib version is only really better if multithreading is an issue. I'll revert it.

[BJ Burg]

Created attachment 265872 [details]
For Landing

[WebKit Commit Bot]

Comment on attachment 265872 [details]
For Landing

Rejecting attachment 265872 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 265872, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/450660

[BJ Burg]

Committed r192641: <http://trac.webkit.org/changeset/192641>