Bug 60555 – Autocorrection persists after deleting and retyping the same word at same location.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Jia Pu]

If user delete the whole corrected word, and re-type the same uncorrected word at the same position, autocorrection will be applied again.
<rdar://problem/9373915>

[Jia Pu]

Created attachment 92968 [details]
Patch (v1)

[Darin Adler]

Comment on attachment 92968 [details]
Patch (v1)

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

Looks good. Please consider my comments.

> Source/WebCore/editing/CompositeEditCommand.cpp:338
> +void CompositeEditCommand::replaceTextInNode(PassRefPtr<Text> node, unsigned offset, unsigned count, const String& replacementText, bool preserveMarkers)

The preserveMarkers behavior completely wrap the old behavior and is not intertwined with it. Given that, we should just add a new function, instead of adding a boolean to the existing function to change its behavior. This will be much clearer at callsites than a mystery "true".

> Source/WebCore/editing/CompositeEditCommand.cpp:341
> +    RefPtr<Text> textNode(node);

The names here are close enough that this is error prone. A more usual naming scheme is to name the argument prpNode and the local variable node.

> Source/WebCore/editing/CompositeEditCommand.cpp:489
> +        replaceTextInNode(textNode, upstream, length, rebalancedString, true);

The meaning of true here is unclear. It would be better to have either two separate functions, or use an enum for this sort of thing. I favor the two separate functions.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:756
> +    VisiblePosition wordStart = startOfWord(startOfSelection, RightWordIfOnBoundary);
> +    if (startOfSelection != wordStart)
> +        return String();

I don’t think the local variable wordStart makes the code easy to read.

Also, it would be better to call a function to check that something is at a start of a word rather than computing the start of the word and explicitly checking !=.

For other units, like lines, we have isStartOfLine functions. I think we should have that for words too.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:763
> +        DocumentMarker marker = markers[i];

Copying into a local variable is unnecessarily inefficient. I suggest using a const& instead.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:764
> +        if ((int)marker.startOffset == startOfSelection.deepEquivalent().offsetInContainerNode())

The typecast here is undesirable. We’d be better off with a local variable and an implicit conversion rather than a typecast. Also, we prefer C++ casts to C-style casts.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:846
> +    Frame* frame = document()->frame();
> +    if (!originalString.isEmpty() && frame)
> +        frame->editor()->deletedAutocorrectionAtPosition(m_endingPosition, originalString);

Seems wasteful to fetch the frame even if originalString is empty.

> Source/WebCore/editing/DeleteSelectionCommand.h:76
> +    // On platforms that support autocorrection panel, if a deletion command deletes
> +    // a correction, we want to keep track of original string replaced by correction.
> +    // So that when user type the original string again, we won't apply the same correction
> +    // again. This function provides access to original string after the correction has
> +    // been deleted.
> +    String originalStringForAutocorrectionAtBeginningOfSelection();

Comment here seems a little too long and redundant with the comment for the DeletedAutocorrection bit. Maybe we can tighten this up a bit.

> Source/WebCore/editing/Editor.cpp:2236
> +            bool existingMarkersPermitReplacement = m_spellingCorrector->processMarkersOnTextToBeReplacedByResult(result, rangeToReplace.get(), replacedString);
>  
> -            // Don't correct spelling in an already-corrected word.
> -            DocumentMarkerController* markers = m_frame->document()->markers();
> -            if (markers->hasMarkers(rangeToReplace.get(), DocumentMarker::Replacement)) {
> -                doReplacement = false;
> -                if (result->type == TextCheckingTypeCorrection)
> -                    m_spellingCorrector->recordSpellcheckerResponseForModifiedCorrection(rangeToReplace.get(), replacedString, result->replacement);
> -            } else if (markers->hasMarkers(rangeToReplace.get(), DocumentMarker::RejectedCorrection))
> -                doReplacement = false;
> -
> -            if (!(shouldPerformReplacement || shouldShowCorrectionPanel) || !doReplacement)
> +            if (!(shouldPerformReplacement || shouldShowCorrectionPanel) || !doReplacement || !existingMarkersPermitReplacement)
>                  continue;

Seems slightly wasteful to compute existingMarkersPermitReplacement even if the other tests are true. I suggest you move the other early exists earlier in the function and only compute replacedString and existingMarkersPermitReplacement after those.

> Source/WebCore/editing/SpellingCorrectionController.cpp:526
> +    if (string.isEmpty() || !isWhitespace(string[string.length()-1]))

Should use spaces around the "-" here. Is the isWhitespace check the correct type of whitespace check? I ask because there are many different whitespace definitions used in different contexts.

> Source/WebCore/editing/SpellingCorrectionController.cpp:553
> +    if (markers.isEmpty())
> +        return true;

No need for this early exit; the code already will efficiently do just this without an explicit check.

> Source/WebCore/editing/SpellingCorrectionController.h:54
>  class TextCheckerClient;
> +struct TextCheckingResult;
>  class VisibleSelection;

The struct should have its own paragraph rather than being sorted in with the classes.

> Source/WebCore/editing/SpellingCorrectionController.h:121
> +    // This function return false, if the replacement should not be carried out.

No need for the comma here.

[Jia Pu]

Created attachment 93000 [details]
Patch (v2)

[Darin Adler]

Comment on attachment 93000 [details]
Patch (v2)

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

> Source/WebCore/dom/DocumentMarker.h:57
> +        // This marker indicate user has deleted an autocorrection starting at the end of the

Should be “This marker indicates”.

> Source/WebCore/dom/DocumentMarker.h:58
> +        // range that bears this marker. In some platforms, if user later inserts the same original

Should be “if the user”.

> Source/WebCore/dom/DocumentMarker.h:60
> +        // marker is the original word before autocorrection is applied.

Should be “was applied”.

> Source/WebCore/editing/CompositeEditCommand.cpp:344
> +void CompositeEditCommand::replaceTextInNodeAndPreserveMarkers(PassRefPtr<Text> prpNode, unsigned offset, unsigned count, const String& replacementText)

This is good. A name I like slightly better is replaceTextInNodePreservingMarkers. I’m OK with your current name or with my suggested name.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:760
> +    if (markers.isEmpty())
> +        return String();

This special case should be removed. I suggested that in the last review at a different call site but somehow missed it here.

> Source/WebCore/editing/visible_units.cpp:316
> +bool isStartOfWord(const VisiblePosition &p)

The "&" should be one character to the left. The name of the local variable should be “position”, not “p”.

> Source/WebCore/editing/visible_units.h:47
> +bool isStartOfWord(const VisiblePosition &);

There should not be a space before the "&".

[Jia Pu]

Created attachment 93013 [details]
Patch (v3)

Updated patch based on comment #4, and merged TOT.

[WebKit Review Bot]

Comment on attachment 93013 [details]
Patch (v3)

Rejecting attachment 93013 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Enrica Casucci]

Could you explain why you don't need to pass true to replaceTextInNode in ReplaceSelectionCommand, InsertTextCommand and in the last occurrence of it in CompositeEditCommand?
It is not clear to me at all.

[Jia Pu]

(In reply to comment #7)
> Could you explain why you don't need to pass true to replaceTextInNode in ReplaceSelectionCommand, InsertTextCommand and in the last occurrence of it in CompositeEditCommand?
> It is not clear to me at all.

I think we probably don't want to preserve the markers if we are replacing non-whitespace characters. For instance, if user copy-and-paste to replace a paragraph, we don't want the new text to bear existing markers, and interfere autocorrection.

So this patch only preserve markers where it seems to me that we're merely rebalancing whitespaces.

[Enrica Casucci]

I understand. What about though the case of ReplaceSelectionCommand::performTrivialReplace or the equivalent InsertTextCommand::performTrivialReplace?

[Jia Pu]

(In reply to comment #9)
> I understand. What about though the case of ReplaceSelectionCommand::performTrivialReplace or the equivalent InsertTextCommand::performTrivialReplace?

My understanding is that they aren't just for replacing whitespaces. If they are, we should replace replaceTextInNode() with replaceTextInNodePreservingMarkers().

With this patch, I'm aiming for improve the experience as much as possible without changing other current behavior. So the change is rather conservative here. 

The issue is quite difficult to tackle. Even with this patch, if user delete the whole autocorrected word AND the whitespace that precedes the word, then type whitespace and the original word again, there isn't much we can do but reapplying the same autocorrection again.

[WebKit Review Bot]

Comment on attachment 93013 [details]
Patch (v3)

Rejecting attachment 93013 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Jia Pu]

Comment on attachment 93013 [details]
Patch (v3)

Try to commit+ again hoping commit queue has picked up the updated committer list.

[WebKit Review Bot]

Comment on attachment 93013 [details]
Patch (v3)

Rejecting attachment 93013 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Jia Pu]

Comment on attachment 93013 [details]
Patch (v3)

Can someone help me to commit+ this patch?

I added my account to committers.py about 4 hours ago, it still didn't get picked up by commit queue.

[WebKit Review Bot]

Comment on attachment 93013 [details]
Patch (v3)

Rejecting attachment 93013 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 93013 [details]
Patch (v3)

Rejecting attachment 93013 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2

Last 500 characters of output:
-sync.html -> failed
....................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers ...........
http/tests/xmlviewer .
http/tests/xmlviewer/dumpAsText ...........
919.33s total testing time

23499 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) timed out
17 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8690390

[WebKit Commit Bot]

Created attachment 93185 [details]
Archive of layout-test-results from cr-jail-8

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-8  Port: Mac  Platform: Mac OS X 10.6.7

[Jia Pu]

Ah, the test file name I added to Skipped is different from the actual file name. Will upload another patch to fix this shortly.

[Jia Pu]

Created attachment 93190 [details]
Patch (v4)

Fixed test failure due to incorrect test file name.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 93190 [details]:

http/tests/appcache/update-cache.html bug 52483 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 93190 [details]
Patch (v4)

Clearing flags on attachment: 93190

Committed r86295: <http://trac.webkit.org/changeset/86295>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

This patch broke Leopard and GTK:

Program terminated with signal 11, Segmentation fault.
#0  0x00002ad09e6c6951 in WebCore::Range::Range (this=0x11bbfb0, ownerDocument=..., startContainer=..., startOffset=2, endContainer=..., endOffset=0) at ../../Source/WebCore/dom/Range.cpp:89
89	    ASSERT(!ec);

Thread 1 (Thread 8980):
#0  0x00002ad09e6c6951 in WebCore::Range::Range (this=0x11bbfb0, ownerDocument=..., startContainer=..., startOffset=2, endContainer=..., endOffset=0) at ../../Source/WebCore/dom/Range.cpp:89
#1  0x00002ad09e6be061 in WebCore::Range::create (ownerDocument=..., start=..., end=...) at ../../Source/WebCore/dom/Range.cpp:99
#2  0x00002ad09e71f951 in WebCore::DeleteSelectionCommand::originalStringForAutocorrectionAtBeginningOfSelection (this=0x11bc400) at ../../Source/WebCore/editing/DeleteSelectionCommand.cpp:757
#3  0x00002ad09e71fb7d in WebCore::DeleteSelectionCommand::doApply (this=0x11bc400) at ../../Source/WebCore/editing/DeleteSelectionCommand.cpp:778
#4  0x00002ad09e720f51 in WebCore::EditCommand::apply (this=0x11bc400) at ../../Source/WebCore/editing/EditCommand.cpp:92
#5  0x00002ad09e70b3c3 in WebCore::CompositeEditCommand::applyCommandToComposite (this=0x11b8350, cmd=...) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:103
#6  0x00002ad09e70d812 in WebCore::CompositeEditCommand::deleteSelection (this=0x11b8350, selection=..., smartDelete=false, mergeBlocksAfterDelete=true, replace=false, expandForSpecialElements=true) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:392
#7  0x00002ad09e78782e in WebCore::TypingCommand::deleteKeyPressed (this=0x11b8350, granularity=WebCore::CharacterGranularity, killRing=false) at ../../Source/WebCore/editing/TypingCommand.cpp:562
#8  0x00002ad09e786165 in WebCore::TypingCommand::doApply (this=0x11b8350) at ../../Source/WebCore/editing/TypingCommand.cpp:295
#9  0x00002ad09e720f51 in WebCore::EditCommand::apply (this=0x11b8350) at ../../Source/WebCore/editing/EditCommand.cpp:92
#10 0x00002ad09e785289 in WebCore::TypingCommand::deleteKeyPressed (document=0xf4f4b0, options=0, granularity=WebCore::CharacterGranularity) at ../../Source/WebCore/editing/TypingCommand.cpp:117
#11 0x00002ad09e731ca4 in WebCore::Editor::deleteWithDirection (this=0x9595a8, direction=WebCore::DirectionBackward, granularity=WebCore::CharacterGranularity, killRing=false, isTypingAction=true) at ../../Source/WebCore/editing/Editor.cpp:319
#12 0x00002ad09e72a572 in WebCore::executeDeleteBackward (frame=0x959000) at ../../Source/WebCore/editing/EditorCommand.cpp:330
#13 0x00002ad09e72e538 in WebCore::Editor::Command::execute (this=0x11b81c0, parameter=..., triggeringEvent=0x0) at ../../Source/WebCore/editing/EditorCommand.cpp:1644
#14 0x00002ad09e30d570 in WebKit::EditorClient::executePendingEditorCommands (this=0x93cc00, frame=0x959000, allowTextInsertion=false) at ../../Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:686
#15 0x00002ad09e30d72e in WebKit::EditorClient::handleKeyboardEvent (this=0x93cc00, event=0x1130240) at ../../Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:720
#16 0x00002ad09e7312f5 in WebCore::Editor::handleKeyboardEvent (this=0x9595a8, event=0x1130240) at ../../Source/WebCore/editing/Editor.cpp:148
#17 0x00002ad09ea26be6 in WebCore::EventHandler::defaultKeyboardEventHandler (this=0x959758, event=0x1130240) at ../../Source/WebCore/page/EventHandler.cpp:2569
#18 0x00002ad09e69f42e in WebCore::Node::defaultEventHandler (this=0xfe53c0, event=0x1130240) at ../../Source/WebCore/dom/Node.cpp:2886
#19 0x00002ad09e67d847 in WebCore::EventDispatcher::dispatchEvent (this=0x7fff8ddd8fc0, event=...) at ../../Source/WebCore/dom/EventDispatcher.cpp:345
#20 0x00002ad09e67bde0 in WebCore::EventDispatchMediator::dispatchEvent (this=0x7fff8ddd9030, dispatcher=0x7fff8ddd8fc0) at ../../Source/WebCore/dom/Event.cpp:320
#21 0x00002ad09e67c1a1 in WebCore::EventDispatcher::dispatchEvent (node=0xfe53c0, mediator=...) at ../../Source/WebCore/dom/EventDispatcher.cpp:60
#22 0x00002ad09e69eb1e in WebCore::Node::dispatchEvent (this=0xfe53c0, event=...) at ../../Source/WebCore/dom/Node.cpp:2802
#23 0x00002ad09e681995 in WebCore::EventTarget::dispatchEvent (this=0xfe53c0, event=..., ec=@0x7fff8ddd916c) at ../../Source/WebCore/dom/EventTarget.cpp:303
#24 0x00002ad09ea26566 in WebCore::EventHandler::keyEvent (this=0x959758, initialKeyEvent=...) at ../../Source/WebCore/page/EventHandler.cpp:2510
#25 0x00002ad09e344185 in webkit_web_view_key_press_event (widget=0x948070, event=0x1101e40) at ../../Source/WebKit/gtk/webkit/webkitwebview.cpp:726
#26 0x00002ad0a0e9fc18 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#27 0x00002ad0a2693289 in g_closure_invoke (closure=0x8e2be0, return_value=0x7fff8ddd94b0, n_param_values=2, param_values=0xf41690, invocation_hint=0x7fff8ddd9470) at /tmp/buildd/glib2.0-2.27.91/./gobject/gclosure.c:767
#28 0x00002ad0a26abff2 in signal_emit_unlocked_R (node=0x8e2c50, detail=<value optimized out>, instance=<value optimized out>, emission_return=<value optimized out>, instance_and_params=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./gobject/gsignal.c:3290
#29 0x00002ad0a26ad97c in g_signal_emit_valist (instance=0x948070, signal_id=<value optimized out>, detail=0, var_args=0x7fff8ddd9660) at /tmp/buildd/glib2.0-2.27.91/./gobject/gsignal.c:2993
#30 0x00002ad0a26ae363 in g_signal_emit (instance=0x7fff8ddd7910, signal_id=0, detail=2758184448) at /tmp/buildd/glib2.0-2.27.91/./gobject/gsignal.c:3040
#31 0x00002ad0a0fb5f3f in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#32 0x00002ad0a0fc641b in gtk_window_propagate_key_event () from /usr/lib/libgtk-x11-2.0.so.0
#33 0x00002ad0a0fc94ab in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#34 0x00002ad0a0e9fc18 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#35 0x00002ad0a269333e in g_closure_invoke (closure=0x8e2be0, return_value=0x7fff8ddd9980, n_param_values=2, param_values=0x10fccf0, invocation_hint=0x7fff8ddd9940) at /tmp/buildd/glib2.0-2.27.91/./gobject/gclosure.c:767
#36 0x00002ad0a26abff2 in signal_emit_unlocked_R (node=0x8e2c50, detail=<value optimized out>, instance=<value optimized out>, emission_return=<value optimized out>, instance_and_params=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./gobject/gsignal.c:3290
#37 0x00002ad0a26ad97c in g_signal_emit_valist (instance=0x8db280, signal_id=<value optimized out>, detail=0, var_args=0x7fff8ddd9b30) at /tmp/buildd/glib2.0-2.27.91/./gobject/gsignal.c:2993
#38 0x00002ad0a26ae363 in g_signal_emit (instance=0x7fff8ddd7910, signal_id=0, detail=2758184448) at /tmp/buildd/glib2.0-2.27.91/./gobject/gsignal.c:3040
#39 0x00002ad0a0fb5f3f in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#40 0x00002ad0a0e981d4 in gtk_propagate_event () from /usr/lib/libgtk-x11-2.0.so.0
#41 0x00002ad0a0e991bb in gtk_main_do_event () from /usr/lib/libgtk-x11-2.0.so.0
#42 0x000000000042835f in dispatchEvent (event=0x1101e40) at ../../Tools/DumpRenderTree/gtk/EventSender.cpp:562
#43 0x0000000000428c06 in keyDownCallback (context=0x2ad0f4012160, function=0x2ad0f4430888, thisObject=0x2ad0f44305e8, argumentCount=2, arguments=0x7fff8ddd9de8, exception=0x7fff8ddd9e88) at ../../Tools/DumpRenderTree/gtk/EventSender.cpp:741
#44 0x00002ad09d55d9b0 in JSC::JSCallbackFunction::call (exec=0x2ad0f4012160) at ../../Source/JavaScriptCore/API/JSCallbackFunction.cpp:67
#45 0x00002ad09d61f2e7 in JSC::cti_op_call_NotJSFunction (args=0x7fff8ddd9fc0) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:2191
#46 0x00002ad09d61a25e in JSC::JITThunks::tryCacheGetByID (callFrame=0x2ad09d6df647, codeBlock=0x7fff8ddd9fa0, returnAddress=..., baseValue=..., propertyName=..., slot=..., stubInfo=0x97a950) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:943
#47 0x00002ad09d5ed683 in JSC::JITCode::execute (this=0x2ad0f4454168, registerFile=0x97cc38, callFrame=0x2ad0f4012038, globalData=0x97a950) at ../../Source/JavaScriptCore/jit/JITCode.h:77
#48 0x00002ad09d5e9ac3 in JSC::Interpreter::execute (this=0x97cc20, program=0x2ad0f4454150, callFrame=0x2ad0f44281d8, scopeChain=0x2ad0f4430150, thisObj=0x2ad0f441c150) at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:770
#49 0x00002ad09d67d456 in JSC::evaluate (exec=0x2ad0f44281d8, scopeChain=0x2ad0f4430150, source=..., thisValue=...) at ../../Source/JavaScriptCore/runtime/Completion.cpp:64
#50 0x00002ad09e474083 in WebCore::JSMainThreadExecState::evaluate (exec=0x2ad0f44281d8, chain=0x2ad0f4430150, source=..., thisValue=...) at ../../Source/WebCore/bindings/js/JSMainThreadExecState.h:54
#51 0x00002ad09e4a4ee6 in WebCore::ScriptController::evaluateInWorld (this=0x9594e8, sourceCode=..., world=0xf56b30) at ../../Source/WebCore/bindings/js/ScriptController.cpp:143
#52 0x00002ad09e4a50a6 in WebCore::ScriptController::evaluate (this=0x9594e8, sourceCode=...) at ../../Source/WebCore/bindings/js/ScriptController.cpp:166
#53 0x00002ad09e6c9a76 in WebCore::ScriptElement::executeScript (this=0xfe4f40, sourceCode=...) at ../../Source/WebCore/dom/ScriptElement.cpp:275
#54 0x00002ad09e6c9702 in WebCore::ScriptElement::prepareScript (this=0xfe4f40, scriptStartPosition=..., supportLegacyTypes=WebCore::ScriptElement::DisallowLegacyTypeInTypeAttribute) at ../../Source/WebCore/dom/ScriptElement.cpp:232
#55 0x00002ad09e85f313 in WebCore::HTMLScriptRunner::runScript (this=0x9ae4a0, script=0xfe4ec0, scriptStartPosition=...) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:296
#56 0x00002ad09e85e984 in WebCore::HTMLScriptRunner::execute (this=0x9ae4a0, scriptElement=..., scriptStartPosition=...) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:170
#57 0x00002ad09e851de9 in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder (this=0x978280) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:205
#58 0x00002ad09e851ea5 in WebCore::HTMLDocumentParser::canTakeNextToken (this=0x978280, mode=WebCore::HTMLDocumentParser::AllowYield, session=...) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:223
#59 0x00002ad09e852247 in WebCore::HTMLDocumentParser::pumpTokenizer (this=0x978280, mode=WebCore::HTMLDocumentParser::AllowYield) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:261
#60 0x00002ad09e851c3e in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible (this=0x978280, mode=WebCore::HTMLDocumentParser::AllowYield) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:175
#61 0x00002ad09e852ca7 in WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution (this=0x978280) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:479
#62 0x00002ad09e852fbb in WebCore::HTMLDocumentParser::notifyFinished (this=0x978280, cachedResource=0xf5a620) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:524
#63 0x00002ad09e95668e in WebCore::CachedResource::checkNotify (this=0xf5a620) at ../../Source/WebCore/loader/cache/CachedResource.cpp:144
#64 0x00002ad09e969615 in WebCore::CachedScript::data (this=0xf5a620, data=..., allDataReceived=true) at ../../Source/WebCore/loader/cache/CachedScript.cpp:104
#65 0x00002ad09e967fa1 in WebCore::CachedResourceRequest::didFinishLoading (this=0xf5b0e0, loader=0xf5ac90) at ../../Source/WebCore/loader/cache/CachedResourceRequest.cpp:166
#66 0x00002ad09e9d10ae in WebCore::SubresourceLoader::didFinishLoading (this=0xf5ac90, finishTime=0) at ../../Source/WebCore/loader/SubresourceLoader.cpp:197
#67 0x00002ad09e9c7f69 in WebCore::ResourceLoader::didFinishLoading (this=0xf5ac90, finishTime=0) at ../../Source/WebCore/loader/ResourceLoader.cpp:436
#68 0x00002ad09ef1666a in WebCore::readCallback (source=0xf3d8c0, asyncResult=0xf65d20, data=0x0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:792
#69 0x00002ad0a23acf65 in async_ready_callback_wrapper (source_object=0xf3d8c0, res=0xf65d20, user_data=0x0) at /tmp/buildd/glib2.0-2.27.91/./gio/ginputstream.c:470
#70 0x00002ad0a23be628 in complete_in_idle_cb_for_thread (_data=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./gio/gsimpleasyncresult.c:812
#71 0x00002ad0a2f25362 in g_main_dispatch (context=0x8dda30) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:2440
#72 g_main_context_dispatch (context=0x8dda30) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3013
#73 0x00002ad0a2f29a28 in g_main_context_iterate (context=0x8dda30, block=<value optimized out>, dispatch=<value optimized out>, self=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3091
#74 0x00002ad0a2f29f35 in g_main_loop_run (loop=0x9b0200) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3299
#75 0x00002ad0a0e99657 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#76 0x0000000000423b7d in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:703
#77 0x000000000042321a in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:495
#78 0x00000000004254dc in main (argc=2, argv=0x7fff8dddb7a8) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1180

[Philippe Normand]

And Windows XP too.
FYI:

Tests that caused the DumpRenderTree tool to crash:

editing/deleting/delete-ligature-001.html	stderr	
editing/deleting/delete-ligature-002.html	stderr	
editing/deleting/delete-ligature-003.html	stderr

[Philippe Normand]

Created attachment 93275 [details]
proposed follow-up fix

[Philippe Normand]

Reopening as a follow-up fix is required.

[Philippe Normand]

(In reply to comment #25)
> Created an attachment (id=93275) [details]
> proposed follow-up fix

Verified this locally on GTK.

[Jia Pu]

(In reply to comment #27)
> (In reply to comment #25)
> > Created an attachment (id=93275) [details] [details]
> > proposed follow-up fix
> 
> Verified this locally on GTK.

Looks like a good safety check to add. Thanks Phillippe.

[Adam Roben (:aroben)]

*** Bug 60699 has been marked as a duplicate of this bug. ***

[Adam Roben (:aroben)]

Comment on attachment 93275 [details]
proposed follow-up fix

r+ to put out the fire on the bots. Hopefully someone with more expertise can look at this more closely after it lands.

[Adam Roben (:aroben)]

More crash logs for posterity:

http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r86300%20(28565)/editing/deleting/delete-ligature-001-crash-log.txt
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r86332%20(30166)/editing/deleting/delete-ligature-001-crash-log.txt
http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r86296%20(16829)/editing/deleting/delete-ligature-001-crash-log.txt

[Philippe Normand]

Committed http://trac.webkit.org/changeset/86339