<rdar://problem/33132405>
Created attachment 316176 [details] Patch
Comment on attachment 316176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316176&action=review > Source/WebKit/ChangeLog:30 > + behavior change since it swaps the order in which key teardown methods are invoked, and as such, incurs > + considerably more risk than just adding null checks. However, that behavior change would fix /all/ callbacks, not just these few... there's nothing that makes this specific to the NSTextInputClient methods, right?
(In reply to Tim Horton from comment #2) > Comment on attachment 316176 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=316176&action=review > > > Source/WebKit/ChangeLog:30 > > + behavior change since it swaps the order in which key teardown methods are invoked, and as such, incurs > > + considerably more risk than just adding null checks. > > However, that behavior change would fix /all/ callbacks, not just these > few... there's nothing that makes this specific to the NSTextInputClient > methods, right? That is true, if NSTextInputContext were to invoke anything else on our WKWebView that dereferences _impl within a NSTextInputClient completion block, we would still crash. However, from the crash logs in the radar, NSTextInputContext only ends up invoking these asynchronous NSTextInputClient methods in the codepath that's registering these crashy completion blocks. In that sense, this is a very targeted fix to deal with NSTextInputContext behavior. I'll take a second look at closing the WebPageProxy at an earlier time. I already tried this approach and confirmed that it fixes the crash without any obvious fallout, but...it should be more thoroughly vetted :P. What do you think?
(In reply to Wenson Hsieh from comment #3) > I'll take a second look at closing the WebPageProxy at an earlier time. I > already tried this approach and confirmed that it fixes the crash without > any obvious fallout, but...it should be more thoroughly vetted :P. Closing the page earlier is both consistent with what we do on iOS and with what we used to do on macOS prior to the introduction of _impl.
(In reply to Wenson Hsieh from comment #3) > (In reply to Tim Horton from comment #2) > > Comment on attachment 316176 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=316176&action=review > > > > > Source/WebKit/ChangeLog:30 > > > + behavior change since it swaps the order in which key teardown methods are invoked, and as such, incurs > > > + considerably more risk than just adding null checks. > > > > However, that behavior change would fix /all/ callbacks, not just these > > few... there's nothing that makes this specific to the NSTextInputClient > > methods, right? > > That is true, if NSTextInputContext were to invoke anything else on our Forget, for a second, about NSTextInputContext. There are lots of other callbacks that use this same mechanism, some which come from the client, all of which could easily call back into something that calls into WebViewImpl. Right? > I'll take a second look at closing the WebPageProxy at an earlier time. I > already tried this approach and confirmed that it fixes the crash without > any obvious fallout, but...it should be more thoroughly vetted :P. > > What do you think? I think I'm with mitz on this one. I made a mistake with WebViewImpl and we should fix it :)
(In reply to Tim Horton from comment #5) > (In reply to Wenson Hsieh from comment #3) > > (In reply to Tim Horton from comment #2) > > > Comment on attachment 316176 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=316176&action=review > > > > > > > Source/WebKit/ChangeLog:30 > > > > + behavior change since it swaps the order in which key teardown methods are invoked, and as such, incurs > > > > + considerably more risk than just adding null checks. > > > > > > However, that behavior change would fix /all/ callbacks, not just these > > > few... there's nothing that makes this specific to the NSTextInputClient > > > methods, right? > > > > That is true, if NSTextInputContext were to invoke anything else on our > > Forget, for a second, about NSTextInputContext. There are lots of other > callbacks that use this same mechanism, some which come from the client, all > of which could easily call back into something that calls into WebViewImpl. > Right? > > > I'll take a second look at closing the WebPageProxy at an earlier time. I > > already tried this approach and confirmed that it fixes the crash without > > any obvious fallout, but...it should be more thoroughly vetted :P. > > > > What do you think? > > I think I'm with mitz on this one. I made a mistake with WebViewImpl and we > should fix it :) Sounds good. I'll update the patch -- thanks, Tim and Dan!
Comment on attachment 316176 [details] Patch Attachment 316176 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4166161 New failing tests: imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Created attachment 316177 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 316180 [details] Close the page earlier
Comment on attachment 316180 [details] Close the page earlier View in context: https://bugs.webkit.org/attachment.cgi?id=316180&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:688 > _page->close(); I wonder if we still have this bug in WKView now?
(In reply to Tim Horton from comment #10) > Comment on attachment 316180 [details] > Close the page earlier > > View in context: > https://bugs.webkit.org/attachment.cgi?id=316180&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:688 > > _page->close(); > > I wonder if we still have this bug in WKView now? I think so, because in -[WKView dealloc], we clear out the WebViewImpl pointer early on by setting `_data->_impl = nullptr;`, which would trigger ~WebViewImpl and then invoke all pending callbacks, which could potentially do things to the WKView that assume _data->_impl exists. We should be able to address this by doing _data->_impl->page().close() before clearing out the _impl pointer.
(In reply to Wenson Hsieh from comment #11) > (In reply to Tim Horton from comment #10) > > Comment on attachment 316180 [details] > > Close the page earlier > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=316180&action=review > > > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:688 > > > _page->close(); > > > > I wonder if we still have this bug in WKView now? > > I think so, because in -[WKView dealloc], we clear out the WebViewImpl > pointer early on by setting `_data->_impl = nullptr;`, which would trigger > ~WebViewImpl and then invoke all pending callbacks, which could potentially > do things to the WKView that assume _data->_impl exists. We should be able > to address this by doing _data->_impl->page().close() before clearing out > the _impl pointer. That being said, can we do it in a followup? I'd like to limit the scope of this change to fixing the crash being tracked in <rdar://problem/33132405>. I filed https://bugs.webkit.org/show_bug.cgi?id=174753 to track this.
Comment on attachment 316180 [details] Close the page earlier Especially since we cannot create a regression test, we want to go beyond fixing this bug and instead structure the code so the bug is highly unlikely to ever return. I think it’s worth some further thought about how to do that after this fix goes in.
Comment on attachment 316180 [details] Close the page earlier Clearing flags on attachment: 316180 Committed r219765: <http://trac.webkit.org/changeset/219765>
All reviewed patches have been landed. Closing bug.
(In reply to Wenson Hsieh from comment #12) > That being said, can we do it in a followup? I'd like to limit the scope of > this change to fixing the crash being tracked in <rdar://problem/33132405>. > I filed https://bugs.webkit.org/show_bug.cgi?id=174753 to track this. Yes, that definitely makes sense.
I think the change log for this is misleading; sorry I did not spot it when reviewing. The comment says "Tweaks -[WKWebView dealloc] to close the WebPageProxy at an earlier time". But the change here is to start calling WebPageProxy::close on Mac *at* *all*. Before this patch, -[WKWebView dealloc] called WebPageProxy::close on iOS but not at all on Mac. As far as I can tell, this has been true since r191907 and I am not sure if the change to omit the call on the Mac was intentional. Tim, you might want to take a look since you made that r191907 change and Anders reviewed it.
(In reply to Darin Adler from comment #17) > I think the change log for this is misleading; sorry I did not spot it when > reviewing. The comment says "Tweaks -[WKWebView dealloc] to close the > WebPageProxy at an earlier time". But the change here is to start calling > WebPageProxy::close on Mac *at* *all*. Before this patch, -[WKWebView > dealloc] called WebPageProxy::close on iOS but not at all on Mac. Before the patch, close would still be called on macOS by ~WebViewImpl, which was invoked beneath -[WKWebView dealloc] (in the call to super).
OK, I see.