51569 2010-12-23 15:50:44 -0800 [GTK] EditorClient::generateEditorCommands queues up "null string" commands 2010-12-27 17:04:51 -0800 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 darin webkit-unassigned abarth eric mrobinson rniwa webkit.review.bot oldest_to_newest 326336 0 darin 2010-12-23 15:50:44 -0800 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. 326882 1 77454 mrobinson 2010-12-26 07:25:56 -0800 Created attachment 77454 Patch for this issue 326915 2 eric 2010-12-26 13:29:40 -0800 How do we test this? 326918 3 rniwa 2010-12-26 13:41:59 -0800 (In reply to comment #2) > How do we test this? Does eventSender use this code path? 326926 4 mrobinson 2010-12-26 14:29:57 -0800 (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. 326933 5 77454 rniwa 2010-12-26 15:32:00 -0800 Comment on attachment 77454 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. 327110 6 77518 mrobinson 2010-12-27 13:53:13 -0800 Created attachment 77518 Simplified version of the previous patch 327111 7 mrobinson 2010-12-27 13:56:42 -0800 (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. 327112 8 mrobinson 2010-12-27 13:57:41 -0800 (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. 327120 9 rniwa 2010-12-27 14:35:56 -0800 (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. 327125 10 77518 mrobinson 2010-12-27 14:48:28 -0800 Comment on attachment 77518 Simplified version of the previous patch Clearing flags on attachment: 77518 Committed r74697: <http://trac.webkit.org/changeset/74697> 327126 11 mrobinson 2010-12-27 14:48:33 -0800 All reviewed patches have been landed. Closing bug. 327156 12 webkit.review.bot 2010-12-27 17:04:51 -0800 http://trac.webkit.org/changeset/74697 might have broken Leopard Intel Debug (Tests) 77454 2010-12-26 07:25:56 -0800 2010-12-27 13:53:09 -0800 Patch for this issue bug-51569-20101226072555.patch text/plain 3100 mrobinson ZGlmZiAtLWdpdCBhL1dlYktpdC9ndGsvQ2hhbmdlTG9nIGIvV2ViS2l0L2d0ay9DaGFuZ2VMb2cK aW5kZXggOGE3ZTk3OWQ1MTc3ODk0MTNiZmQ5MmJjYjY2MmMwOWFjMTIzMzVmYi4uYTYxNjgwNjI1 ZmI4ZTdiNThjNDdhNDhjZWYyMmUzMTBjZjVkNTAwMCAxMDA2NDQKLS0tIGEvV2ViS2l0L2d0ay9D aGFuZ2VMb2cKKysrIGIvV2ViS2l0L2d0ay9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMSBAQAorMjAx MC0xMi0yNiAgTWFydGluIFJvYmluc29uICA8bXJvYmluc29uQGlnYWxpYS5jb20+CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW0dUS10gRWRpdG9yQ2xp ZW50OjpnZW5lcmF0ZUVkaXRvckNvbW1hbmRzIHF1ZXVlcyB1cCAibnVsbCBzdHJpbmciIGNvbW1h bmRzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01MTU2 OQorCisgICAgICAgIFByZXZlbnQgYWRkaW5nICJudWxsIHN0cmluZyIgZWRpdG9yIGNvbW1hbmRz IGJ5IGZpcnN0IGNoZWNraW5nIHdoZXRoZXIgb3Igbm90CisgICAgICAgIHRoZSBjb21tYW5kIHN0 cmluZyBpcyBudWxsIGluIGdlbmVyYXRlRWRpdG9yQ29tbWFuZHMgYmVmb3JlIGFwcGVuZGluZyBp dCB0byB0aGUKKyAgICAgICAgbGlzdCBvZiBwZW5kaW5nIGVkaXRvciBjb21tYW5kIHN0cmluZ3Mu CisKKyAgICAgICAgKiBXZWJDb3JlU3VwcG9ydC9FZGl0b3JDbGllbnRHdGsuY3BwOgorICAgICAg ICAoV2ViS2l0OjpFZGl0b3JDbGllbnQ6OmdlbmVyYXRlRWRpdG9yQ29tbWFuZHMpOiBDaGVjayBm b3IgdGhlIG51bGwgc3RyaW5nIChubworICAgICAgICBjb21tYW5kIGZvciB0aGlzIGtleSBjb21i aW5hdGlvbikgYmVmb3JlIGFwcGVuZGluZyBhIGNvbW1hbmQgc3RyaW5nIHRvIHRoZSBsaXN0IG9m CisgICAgICAgIHBlbmRpbmcgZWRpdG9yIGNvbW1hbmRzLgorICAgICAgICAoV2ViS2l0OjpFZGl0 b3JDbGllbnQ6OmV4ZWN1dGVQZW5kaW5nRWRpdG9yQ29tbWFuZHMpOiBBZGQgYW4gQVNTRVJUIHdo aWNoIGRldGVjdHMKKyAgICAgICAgbnVsbCBjb21tYW5kIHN0cmluZ3MuCisKIDIwMTAtMTItMjEg IFBoaWxpcHBlIE5vcm1hbmQgIDxwbm9ybWFuZEBpZ2FsaWEuY29tPgogCiAgICAgICAgIFJldmll d2VkIGJ5IEVyaWMgU2VpZGVsLgpkaWZmIC0tZ2l0IGEvV2ViS2l0L2d0ay9XZWJDb3JlU3VwcG9y dC9FZGl0b3JDbGllbnRHdGsuY3BwIGIvV2ViS2l0L2d0ay9XZWJDb3JlU3VwcG9ydC9FZGl0b3JD bGllbnRHdGsuY3BwCmluZGV4IDA1MjJmODcxZTQ5MGRhMjE1MzIwZGEyMzE0OTcwOWUzYWJjNTNl MmEuLjliM2Y0ZWM2NDg0MTIxYzgyOTA0YzY0OTc0NzIyN2E0Y2FlNTgxMGUgMTAwNjQ0Ci0tLSBh L1dlYktpdC9ndGsvV2ViQ29yZVN1cHBvcnQvRWRpdG9yQ2xpZW50R3RrLmNwcAorKysgYi9XZWJL aXQvZ3RrL1dlYkNvcmVTdXBwb3J0L0VkaXRvckNsaWVudEd0ay5jcHAKQEAgLTY1MiwyNCArNjUy LDMzIEBAIHZvaWQgRWRpdG9yQ2xpZW50OjpnZW5lcmF0ZUVkaXRvckNvbW1hbmRzKGNvbnN0IEtl eWJvYXJkRXZlbnQqIGV2ZW50KQogICAgIGlmIChldmVudC0+Y3RybEtleSgpKQogICAgICAgICBt b2RpZmllcnMgfD0gQ3RybEtleTsKIAotCisgICAgaW50IG1hcEtleSA9IDA7CisgICAgSGFzaE1h cDxpbnQsIGNvbnN0IGNoYXIqPiogY29tbWFuZE1hcCA9IDA7CiAgICAgaWYgKGV2ZW50LT50eXBl KCkgPT0gZXZlbnROYW1lcygpLmtleWRvd25FdmVudCkgewotICAgICAgICBpbnQgbWFwS2V5ID0g bW9kaWZpZXJzIDw8IDE2IHwgZXZlbnQtPmtleUNvZGUoKTsKLSAgICAgICAgaWYgKG1hcEtleSkK LSAgICAgICAgICAgIG1fcGVuZGluZ0VkaXRvckNvbW1hbmRzLmFwcGVuZChrZXlEb3duQ29tbWFu ZHNNYXAuZ2V0KG1hcEtleSkpOwotICAgICAgICByZXR1cm47CisgICAgICAgIG1hcEtleSA9IG1v ZGlmaWVycyA8PCAxNiB8IGV2ZW50LT5rZXlDb2RlKCk7CisgICAgICAgIGNvbW1hbmRNYXAgPSAm a2V5RG93bkNvbW1hbmRzTWFwOworICAgIH0gZWxzZSB7CisgICAgICAgIEFTU0VSVChldmVudC0+ dHlwZSgpID09IGV2ZW50TmFtZXMoKS5rZXlwcmVzc0V2ZW50KTsKKyAgICAgICAgbWFwS2V5ID0g bW9kaWZpZXJzIDw8IDE2IHwgZXZlbnQtPmNoYXJDb2RlKCk7CisgICAgICAgIGNvbW1hbmRNYXAg PSAma2V5UHJlc3NDb21tYW5kc01hcDsKICAgICB9CiAKLSAgICBpbnQgbWFwS2V5ID0gbW9kaWZp ZXJzIDw8IDE2IHwgZXZlbnQtPmNoYXJDb2RlKCk7Ci0gICAgaWYgKG1hcEtleSkKLSAgICAgICAg bV9wZW5kaW5nRWRpdG9yQ29tbWFuZHMuYXBwZW5kKGtleVByZXNzQ29tbWFuZHNNYXAuZ2V0KG1h cEtleSkpOworICAgICBpZiAoIW1hcEtleSkKKyAgICAgICAgcmV0dXJuOworCisgICAgIGNvbnN0 IGNoYXIqIGNvbW1hbmRTdHJpbmcgPSBjb21tYW5kTWFwLT5nZXQobWFwS2V5KTsKKyAgICAgaWYg KCFjb21tYW5kU3RyaW5nKQorICAgICAgICByZXR1cm47CisgICAgIG1fcGVuZGluZ0VkaXRvckNv bW1hbmRzLmFwcGVuZChjb21tYW5kU3RyaW5nKTsKIH0KIAogYm9vbCBFZGl0b3JDbGllbnQ6OmV4 ZWN1dGVQZW5kaW5nRWRpdG9yQ29tbWFuZHMoRnJhbWUqIGZyYW1lLCBib29sIGFsbG93VGV4dElu c2VydGlvbikKIHsKICAgICBWZWN0b3I8RWRpdG9yOjpDb21tYW5kPiBjb21tYW5kczsKICAgICBm b3IgKHNpemVfdCBpID0gMDsgaSA8IG1fcGVuZGluZ0VkaXRvckNvbW1hbmRzLnNpemUoKTsgaSsr KSB7Ci0gICAgICAgIEVkaXRvcjo6Q29tbWFuZCBjb21tYW5kID0gZnJhbWUtPmVkaXRvcigpLT5j b21tYW5kKG1fcGVuZGluZ0VkaXRvckNvbW1hbmRzLmF0KGkpKTsKKyAgICAgICAgY29uc3QgY2hh ciogY29tbWFuZFN0cmluZyA9IG1fcGVuZGluZ0VkaXRvckNvbW1hbmRzLmF0KGkpOworICAgICAg ICBBU1NFUlQoY29tbWFuZFN0cmluZyk7CisgICAgICAgIEVkaXRvcjo6Q29tbWFuZCBjb21tYW5k ID0gZnJhbWUtPmVkaXRvcigpLT5jb21tYW5kKGNvbW1hbmRTdHJpbmcpOwogICAgICAgICBpZiAo Y29tbWFuZC5pc1RleHRJbnNlcnRpb24oKSAmJiAhYWxsb3dUZXh0SW5zZXJ0aW9uKQogICAgICAg ICAgICAgcmV0dXJuIGZhbHNlOwogCg== 77518 2010-12-27 13:53:13 -0800 2010-12-27 14:48:28 -0800 Simplified version of the previous patch bug-51569-20101227135309.patch text/plain 2968 mrobinson ZGlmZiAtLWdpdCBhL1dlYktpdC9ndGsvQ2hhbmdlTG9nIGIvV2ViS2l0L2d0ay9DaGFuZ2VMb2cK aW5kZXggODg5ZjE4ZTA0MDFkMzk2NGU2MDRkNTljM2Y3MmU4ZjczMjgyNDE5Mi4uYzY5M2FjMDMz MTMwMDM1M2E2ODY3NzAwOGQ1MDI0MzA4NGViYjM2MyAxMDA2NDQKLS0tIGEvV2ViS2l0L2d0ay9D aGFuZ2VMb2cKKysrIGIvV2ViS2l0L2d0ay9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMSBAQAorMjAx MC0xMi0yNiAgTWFydGluIFJvYmluc29uICA8bXJvYmluc29uQGlnYWxpYS5jb20+CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW0dUS10gRWRpdG9yQ2xp ZW50OjpnZW5lcmF0ZUVkaXRvckNvbW1hbmRzIHF1ZXVlcyB1cCAibnVsbCBzdHJpbmciIGNvbW1h bmRzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01MTU2 OQorCisgICAgICAgIFByZXZlbnQgYWRkaW5nICJudWxsIHN0cmluZyIgZWRpdG9yIGNvbW1hbmRz IGJ5IGZpcnN0IGNoZWNraW5nIHdoZXRoZXIgb3Igbm90CisgICAgICAgIHRoZSBjb21tYW5kIHN0 cmluZyBpcyBudWxsIGluIGdlbmVyYXRlRWRpdG9yQ29tbWFuZHMgYmVmb3JlIGFwcGVuZGluZyBp dCB0byB0aGUKKyAgICAgICAgbGlzdCBvZiBwZW5kaW5nIGVkaXRvciBjb21tYW5kIHN0cmluZ3Mu CisKKyAgICAgICAgKiBXZWJDb3JlU3VwcG9ydC9FZGl0b3JDbGllbnRHdGsuY3BwOgorICAgICAg ICAoV2ViS2l0OjpFZGl0b3JDbGllbnQ6OmdlbmVyYXRlRWRpdG9yQ29tbWFuZHMpOiBDaGVjayBm b3IgdGhlIG51bGwgc3RyaW5nIChubworICAgICAgICBjb21tYW5kIGZvciB0aGlzIGtleSBjb21i aW5hdGlvbikgYmVmb3JlIGFwcGVuZGluZyBhIGNvbW1hbmQgc3RyaW5nIHRvIHRoZSBsaXN0IG9m CisgICAgICAgIHBlbmRpbmcgZWRpdG9yIGNvbW1hbmRzLgorICAgICAgICAoV2ViS2l0OjpFZGl0 b3JDbGllbnQ6OmV4ZWN1dGVQZW5kaW5nRWRpdG9yQ29tbWFuZHMpOiBBZGQgYW4gQVNTRVJUIHdo aWNoIGRldGVjdHMKKyAgICAgICAgbnVsbCBjb21tYW5kIHN0cmluZ3MuCisKIDIwMTAtMTItMjcg IFhhbiBMb3BleiAgPHhsb3BlekBpZ2FsaWEuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IE1h cnRpbiBSb2JpbnNvbi4KZGlmZiAtLWdpdCBhL1dlYktpdC9ndGsvV2ViQ29yZVN1cHBvcnQvRWRp dG9yQ2xpZW50R3RrLmNwcCBiL1dlYktpdC9ndGsvV2ViQ29yZVN1cHBvcnQvRWRpdG9yQ2xpZW50 R3RrLmNwcAppbmRleCAwNTIyZjg3MWU0OTBkYTIxNTMyMGRhMjMxNDk3MDllM2FiYzUzZTJhLi4z ZjU1Y2NhNjhjMzEzYThkNzIxNDgwY2I5NTRjNWVjYjE2M2JiNTVkIDEwMDY0NAotLS0gYS9XZWJL aXQvZ3RrL1dlYkNvcmVTdXBwb3J0L0VkaXRvckNsaWVudEd0ay5jcHAKKysrIGIvV2ViS2l0L2d0 ay9XZWJDb3JlU3VwcG9ydC9FZGl0b3JDbGllbnRHdGsuY3BwCkBAIC02NTIsMjQgKzY1MiwyMyBA QCB2b2lkIEVkaXRvckNsaWVudDo6Z2VuZXJhdGVFZGl0b3JDb21tYW5kcyhjb25zdCBLZXlib2Fy ZEV2ZW50KiBldmVudCkKICAgICBpZiAoZXZlbnQtPmN0cmxLZXkoKSkKICAgICAgICAgbW9kaWZp ZXJzIHw9IEN0cmxLZXk7CiAKLQotICAgIGlmIChldmVudC0+dHlwZSgpID09IGV2ZW50TmFtZXMo KS5rZXlkb3duRXZlbnQpIHsKLSAgICAgICAgaW50IG1hcEtleSA9IG1vZGlmaWVycyA8PCAxNiB8 IGV2ZW50LT5rZXlDb2RlKCk7Ci0gICAgICAgIGlmIChtYXBLZXkpCi0gICAgICAgICAgICBtX3Bl bmRpbmdFZGl0b3JDb21tYW5kcy5hcHBlbmQoa2V5RG93bkNvbW1hbmRzTWFwLmdldChtYXBLZXkp KTsKKyAgICAvLyBGb3Iga2V5cHJlc3MgZXZlbnRzLCB3ZSB3YW50IGNoYXJDb2RlKCksIGJ1dCBr ZXlDb2RlKCkgZG9lcyB0aGF0LgorICAgIGludCBtYXBLZXkgPSBtb2RpZmllcnMgPDwgMTYgfCBl dmVudC0+a2V5Q29kZSgpOworICAgIGlmICghbWFwS2V5KQogICAgICAgICByZXR1cm47Ci0gICAg fQotCi0gICAgaW50IG1hcEtleSA9IG1vZGlmaWVycyA8PCAxNiB8IGV2ZW50LT5jaGFyQ29kZSgp OwotICAgIGlmIChtYXBLZXkpCi0gICAgICAgIG1fcGVuZGluZ0VkaXRvckNvbW1hbmRzLmFwcGVu ZChrZXlQcmVzc0NvbW1hbmRzTWFwLmdldChtYXBLZXkpKTsKKyAgICBIYXNoTWFwPGludCwgY29u c3QgY2hhcio+KiBjb21tYW5kTWFwID0gZXZlbnQtPnR5cGUoKSA9PSBldmVudE5hbWVzKCkua2V5 ZG93bkV2ZW50ID8KKyAgICAgICAgJmtleURvd25Db21tYW5kc01hcCA6ICZrZXlQcmVzc0NvbW1h bmRzTWFwOworICAgIGlmIChjb25zdCBjaGFyKiBjb21tYW5kU3RyaW5nID0gY29tbWFuZE1hcC0+ Z2V0KG1hcEtleSkpCisgICAgICAgIG1fcGVuZGluZ0VkaXRvckNvbW1hbmRzLmFwcGVuZChjb21t YW5kU3RyaW5nKTsKIH0KIAogYm9vbCBFZGl0b3JDbGllbnQ6OmV4ZWN1dGVQZW5kaW5nRWRpdG9y Q29tbWFuZHMoRnJhbWUqIGZyYW1lLCBib29sIGFsbG93VGV4dEluc2VydGlvbikKIHsKICAgICBW ZWN0b3I8RWRpdG9yOjpDb21tYW5kPiBjb21tYW5kczsKICAgICBmb3IgKHNpemVfdCBpID0gMDsg aSA8IG1fcGVuZGluZ0VkaXRvckNvbW1hbmRzLnNpemUoKTsgaSsrKSB7Ci0gICAgICAgIEVkaXRv cjo6Q29tbWFuZCBjb21tYW5kID0gZnJhbWUtPmVkaXRvcigpLT5jb21tYW5kKG1fcGVuZGluZ0Vk aXRvckNvbW1hbmRzLmF0KGkpKTsKKyAgICAgICAgY29uc3QgY2hhciogY29tbWFuZFN0cmluZyA9 IG1fcGVuZGluZ0VkaXRvckNvbW1hbmRzLmF0KGkpOworICAgICAgICBBU1NFUlQoY29tbWFuZFN0 cmluZyk7CisgICAgICAgIEVkaXRvcjo6Q29tbWFuZCBjb21tYW5kID0gZnJhbWUtPmVkaXRvcigp LT5jb21tYW5kKGNvbW1hbmRTdHJpbmcpOwogICAgICAgICBpZiAoY29tbWFuZC5pc1RleHRJbnNl cnRpb24oKSAmJiAhYWxsb3dUZXh0SW5zZXJ0aW9uKQogICAgICAgICAgICAgcmV0dXJuIGZhbHNl OwogCg==