Bug 62248 – WKResponderChainSink mechanism isn't robust against some types of responder chain manipulation

• Component: WebKit2
• Status: RESOLVED FIXED

[John Sullivan]

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.

[John Sullivan]

Created attachment 96326 [details]
Patch to make [WKResponderChainSink detach] robust against some kinds of responder chain manipulations

[Darin Adler]

Comment on attachment 96326 [details]
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.

[Darin Adler]

Patch is OK as is.

[John Sullivan]

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.

[John Sullivan]

Fixed in http://trac.webkit.org/changeset/88298