Bug 51569 – [GTK] EditorClient::generateEditorCommands queues up "null string" commands

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Adler]

After <http://trac.webkit.org/changeset/74580>, we saw assertion failures on GTK.

The cause seems to be EditorClient::generateEditorCommands calling m_pendingEditorCommands.append with a null pointer, and then later turning that into a command name string. Best fix is probably to check for null before appending to the pending commands vector.

[Martin Robinson]

Created attachment 77454 [details]
Patch for this issue

[Eric Seidel (no email)]

How do we test this?

[Ryosuke Niwa]

(In reply to comment #2)
> How do we test this?

Does eventSender use this code path?

[Martin Robinson]

(In reply to comment #2)
> How do we test this?

Recently a null command string would cause an assertion failure deeper in the editing code on many tests (> 20 crashes on the debug build before it ).

In this patch, I added an assertion in the WebKitGTK+ specific code which ensures that no null editing command strings are generated. If this ever starts failing again, many tests will start hitting this assertion and show crashes on the bots. 

If this isn't sufficient, it will be pretty easy to create a test that verifies this. All keystrokes that don't result in an editor command were generating a null command string, which is why so many tests will start crashing.

[Ryosuke Niwa]

Comment on attachment 77454 [details]
Patch for this issue

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

> WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:666
> +     if (!mapKey)

When is this condition ever true?  I would have put an assertion here unless you know a condition in which map key is 0.

[Martin Robinson]

Created attachment 77518 [details]
Simplified version of the previous patch

[Martin Robinson]

(In reply to comment #5)
> When is this condition ever true?  I would have put an assertion here unless you know a condition in which map key is 0.

IIRC, some misbehaving GTK+ input methods (which are essentially separate programs) may generate key events with node keyval value (which is used for the keyCode() later on). I wasn't able to reproduce this, but I think not trusting the input method code is the right decision here.

[Martin Robinson]

(In reply to comment #6)
> Created an attachment (id=77518) [details]
> Simplified version of the previous patch

I've attached a simplified version of the previous patch. It seems keyCode() will call charCode() automatically for keypress events, so it isn't necessary to special-case it.

[Ryosuke Niwa]

(In reply to comment #7)
> (In reply to comment #5)
> > When is this condition ever true?  I would have put an assertion here unless you know a condition in which map key is 0.
> 
> IIRC, some misbehaving GTK+ input methods (which are essentially separate programs) may generate key events with node keyval value (which is used for the keyCode() later on). I wasn't able to reproduce this, but I think not trusting the input method code is the right decision here.

Ah, ok. Sounds good to me.

(In reply to comment #8)
> (In reply to comment #6)
> > Created an attachment (id=77518) [details] [details]
> > Simplified version of the previous patch
> 
> I've attached a simplified version of the previous patch. It seems keyCode() will call charCode() automatically for keypress events, so it isn't necessary to special-case it.

Ok.  I trust you on this.

[Martin Robinson]

Comment on attachment 77518 [details]
Simplified version of the previous patch

Clearing flags on attachment: 77518

Committed r74697: <http://trac.webkit.org/changeset/74697>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74697 might have broken Leopard Intel Debug (Tests)