62248 2011-06-07 16:08:35 -0700 WKResponderChainSink mechanism isn't robust against some types of responder chain manipulation 2011-06-07 17:45:07 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 sullivan sullivan darin oldest_to_newest 416894 0 sullivan 2011-06-07 16:08:35 -0700 WebKit2's WKResponderChainSink mechanism assumes that no other code has modified the responder chain during the lifetime of this WKResponderChainSink. That's generally a decent assumption, because the lifetime of these objects (as currently used) is wrapped around a single call to [super doCommandBySelector:]. But if that call ends up modifying the responder chain, [WKResponderChainSink detach] can modify it again in the wrong way, leaving itself in the responder chain even after it has been dealloc'ed. 416913 1 96326 sullivan 2011-06-07 16:27:52 -0700 Created attachment 96326 Patch to make [WKResponderChainSink detach] robust against some kinds of responder chain manipulations 416923 2 96326 darin 2011-06-07 16:39:15 -0700 Comment on attachment 96326 Patch to make [WKResponderChainSink detach] robust against some kinds of responder chain manipulations View in context: https://bugs.webkit.org/attachment.cgi?id=96326&action=review > Source/WebKit2/UIProcess/API/mac/WKView.mm:2527 > + // This assumes that the responder chain was either unmodified since > + // -initWithResponderChain: was called, or was modified in such a way > + // that _lastResponderInChain is still in the chain, and self was not > + // moved earlier in the chain than _lastResponderInChain. Another technique would be to start with -[NSWindow firstResponder] and search for self in that chain. We could do that instead of saving _lastResponderInChain. Or we could do both to make sure we’re removed if either technique finds us. 416924 3 darin 2011-06-07 16:39:25 -0700 Patch is OK as is. 416948 4 sullivan 2011-06-07 17:21:17 -0700 I thought about starting with the window’s first responder, but that would be a bigger change for what I needed to work here, and still not guaranteed to find every possible kind of responder chain manipulation. Note that the WKResponderChainSink is not an NSView so it isn’t associated with a window. So to start with the window’s first responder would require that the WKResponderChainSink keeps track of either a window or an NSView. It could be changed to require an NSView rather than an NSResponder in its init method, and then store that NSView and use it to get the window and then start with the first responder in -detach. That would be a little more work in detach since it would have to walk (nearly) the entire responder chain every time instead of just calling nextResponder once in the common case as it does with my patch. As you say, the combination of the two approaches would be more robust, but seemed a little like overkill for now. 416970 5 sullivan 2011-06-07 17:45:07 -0700 Fixed in http://trac.webkit.org/changeset/88298 96326 2011-06-07 16:27:52 -0700 2011-06-07 16:39:15 -0700 Patch to make [WKResponderChainSink detach] robust against some kinds of responder chain manipulations 62248_patch.txt text/plain 2312 sullivan SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDg4MjgwKQorKysgU291cmNlL1dlYktpdDIvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjAgQEAKKzIwMTEtMDYtMDcgIEpvaG4gU3Vs bGl2YW4gIDxzdWxsaXZhbkBhcHBsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgPGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD02MjI0OD4KKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzk1Njg1MTY+CisgICAgICAgIFdL UmVzcG9uZGVyQ2hhaW5TaW5rIG1lY2hhbmlzbSBpc24ndCByb2J1c3QgYWdhaW5zdCBzb21lIHR5 cGVzIG9mIHJlc3BvbmRlciBjaGFpbiBtYW5pcHVsYXRpb24KKworICAgICAgICAqIFVJUHJvY2Vz cy9BUEkvbWFjL1dLVmlldy5tbToKKyAgICAgICAgKC1bV0tSZXNwb25kZXJDaGFpblNpbmsgZGV0 YWNoXSk6CisgICAgICAgIFRoaXMgbWV0aG9kIGZvcm1lcmx5IGJsaW5kbHkgYXNzdW1lZCB0aGF0 IHNpbmNlIC1pbml0V2l0aFJlc3BvbmRlckNoYWluOiBwdXQgc2VsZiBhdCB0aGUKKyAgICAgICAg ZW5kIG9mIHRoZSByZXNwb25kZXIgY2hhaW4sIGFmdGVyIF9sYXN0UmVzcG9uZGVySW5DaGFpbiwg dGhlbiBzZWxmIGlzIHN0aWxsIGF0IHRoZSBlbmQKKyAgICAgICAgb2YgdGhlIHJlc3BvbmRlciBj aGFpbiBhbmQgc3RpbGwgaW1tZWRpYXRlbHkgYWZ0ZXIgX2xhc3RSZXNwb25kZXJJbkNoYWluLiBN YWRlIHRoaXMgZnVuY3Rpb24KKyAgICAgICAgcm9idXN0IGFnYWluc3Qgc29tZSBraW5kcyBvZiBy ZXNwb25kZXIgY2hhaW4gbWFuaXB1bGF0aW9ucywgdGhvdWdoIGl0IGNhbid0IGJlIHJvYnVzdCBh Z2FpbnN0CisgICAgICAgIHNvbWUgb3RoZXIga2luZHMgKGUuZy4sIG1hbmlwdWxhdGlvbnMgdGhh dCByZW1vdmVkIHNlbGYgZnJvbSB0aGlzIGNoYWluIGFuZCBwdXQgaXQgaW50byBzb21lCisgICAg ICAgIG90aGVyIGNoYWluKS4KKwogMjAxMS0wNi0wNyAgWmFsYW4gQnVqdGFzICA8emJ1anRhc0Bn bWFpbC5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQW5kcmVhcyBLbGluZy4KSW5kZXg6IFNv dXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvbWFjL1dLVmlldy5tbQo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBT b3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL21hYy9XS1ZpZXcubW0JKHJldmlzaW9uIDg3NzY4 KQorKysgU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9tYWMvV0tWaWV3Lm1tCSh3b3JraW5n IGNvcHkpCkBAIC0yNTIxLDcgKzI1MjEsMjAgQEAgLSAoaWQpaW5pdFdpdGhSZXNwb25kZXJDaGFp bjooTlNSZXNwb25kZQogCiAtICh2b2lkKWRldGFjaAogewotICAgIFtfbGFzdFJlc3BvbmRlcklu Q2hhaW4gc2V0TmV4dFJlc3BvbmRlcjpuaWxdOworICAgIC8vIFRoaXMgYXNzdW1lcyB0aGF0IHRo ZSByZXNwb25kZXIgY2hhaW4gd2FzIGVpdGhlciB1bm1vZGlmaWVkIHNpbmNlCisgICAgLy8gLWlu aXRXaXRoUmVzcG9uZGVyQ2hhaW46IHdhcyBjYWxsZWQsIG9yIHdhcyBtb2RpZmllZCBpbiBzdWNo IGEgd2F5CisgICAgLy8gdGhhdCBfbGFzdFJlc3BvbmRlckluQ2hhaW4gaXMgc3RpbGwgaW4gdGhl IGNoYWluLCBhbmQgc2VsZiB3YXMgbm90CisgICAgLy8gbW92ZWQgZWFybGllciBpbiB0aGUgY2hh aW4gdGhhbiBfbGFzdFJlc3BvbmRlckluQ2hhaW4uCisgICAgTlNSZXNwb25kZXIgKnJlc3BvbmRl ckJlZm9yZVNlbGYgPSBfbGFzdFJlc3BvbmRlckluQ2hhaW47ICAgIAorICAgIE5TUmVzcG9uZGVy ICpuZXh0ID0gW3Jlc3BvbmRlckJlZm9yZVNlbGYgbmV4dFJlc3BvbmRlcl07CisgICAgZm9yICg7 IG5leHQgJiYgbmV4dCAhPSBzZWxmOyBuZXh0ID0gW25leHQgbmV4dFJlc3BvbmRlcl0pCisgICAg ICAgIHJlc3BvbmRlckJlZm9yZVNlbGYgPSBuZXh0OworICAgIAorICAgIC8vIE5vdGhpbmcgdG8g YmUgZG9uZSBpZiB3ZSBhcmUgbm8gbG9uZ2VyIGluIHRoZSByZXNwb25kZXIgY2hhaW4uCisgICAg aWYgKG5leHQgIT0gc2VsZikKKyAgICAgICAgcmV0dXJuOworICAgIAorICAgIFtyZXNwb25kZXJC ZWZvcmVTZWxmIHNldE5leHRSZXNwb25kZXI6W3NlbGYgbmV4dFJlc3BvbmRlcl1dOwogICAgIF9s YXN0UmVzcG9uZGVySW5DaGFpbiA9IG5pbDsKIH0KIAo=