Deleting a Chinese character typed using pinyin on Mac on the same line as an image causes Safari to crash.
<rdar://problem/27951933>
Created attachment 297066 [details] Fixes the crash
Comment on attachment 297066 [details] Fixes the crash View in context: https://bugs.webkit.org/attachment.cgi?id=297066&action=review > Source/WebKit2/ChangeLog:11 > + Fixed it buy omitting the image as done in r176412 since encoding NSFileWrapper, etc... would require Nit - buy => by
Comment on attachment 297066 [details] Fixes the crash View in context: https://bugs.webkit.org/attachment.cgi?id=297066&action=review > ChangeLog:8 > + Added a manual test. I tried but I couldn't make textInputController to trigger the crash. Very surprising! textInputController can do everything that an input method does. > Source/WebKit2/ChangeLog:12 > + quite a bit of work, and IME doesn't really need to see the image in its attributed string. Is the change in string length and character offsets observable?
(In reply to comment #4) > Comment on attachment 297066 [details] > Fixes the crash > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297066&action=review > > > ChangeLog:8 > > + Added a manual test. I tried but I couldn't make textInputController to trigger the crash. > > Very surprising! textInputController can do everything that an input method > does. I think Chinese IME somehow calls into WKWebView via one of the standard AppKit NSView selectors. > > Source/WebKit2/ChangeLog:12 > > + quite a bit of work, and IME doesn't really need to see the image in its attributed string. > > Is the change in string length and character offsets observable? I can’t think of a way.
Comment on attachment 297066 [details] Fixes the crash View in context: https://bugs.webkit.org/attachment.cgi?id=297066&action=review > Source/WebKit2/ChangeLog:15 > + Also converted assertions in typeFromObject and encode inside ArgumentCodersMac to release assertions > + to crash early inside a WebProcess instead of killing the entire UIProcess for a similar bug in the future. I don't think this is the right fix for this part of the problem: the UI process should guard against this crash (and kill the offending Web process), not the Web process (otherwise, a compromised Web process could still cause this).
(In reply to comment #6) > Comment on attachment 297066 [details] > Fixes the crash > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297066&action=review > > > Source/WebKit2/ChangeLog:15 > > + Also converted assertions in typeFromObject and encode inside ArgumentCodersMac to release assertions > > + to crash early inside a WebProcess instead of killing the entire UIProcess for a similar bug in the future. > > I don't think this is the right fix for this part of the problem: the UI > process should guard against this crash (and kill the offending Web > process), not the Web process (otherwise, a compromised Web process could > still cause this). Okay, I’d just revert this part of the change then since I’m not familiar with IPC / WebKit2 enough to make that change.
Created attachment 297160 [details] Updated per review comments
> I think Chinese IME somehow calls into WKWebView via one of the standard AppKit NSView selectors. An input method can't do that, they are out of process and limited by API provided to them. AppKit itself can, of course. If it's something it does, the right thing to do it to make that testable. Additionally, commands can be sent via -doCommandBySelector: - this is not quite the same as calling WebHTMLView methods directly, but could be close enough to trigger the crash.
(In reply to comment #9) > > I think Chinese IME somehow calls into WKWebView via one of the standard AppKit NSView selectors. > > An input method can't do that, they are out of process and limited by API > provided to them. AppKit itself can, of course. If it's something it does, > the right thing to do it to make that testable. > > Additionally, commands can be sent via -doCommandBySelector: - this is not > quite the same as calling WebHTMLView methods directly, but could be close > enough to trigger the crash. In that case, I think it's probably better if someone who knows about AppKit worked on this bug. I really don't know how to do that.
It turns out that we can just call attributedSubstringForProposedRange on WKWebView and it’d reproduce the crash since the code doesn’t depend on any states related to input methods. After a whole day of fiddling with the exact condition, I’ve successfully made a WebKit API test.
Created attachment 297380 [details] Added a API test
Created attachment 297381 [details] Added a API test
Comment on attachment 297381 [details] Added a API test Test looks good to me!
Created attachment 297385 [details] Patch for landing
Comment on attachment 297385 [details] Patch for landing Clearing flags on attachment: 297385 Committed r209957: <http://trac.webkit.org/changeset/209957>
All reviewed patches have been landed. Closing bug.