Bug 165839 – Deleting a character converted from pinyin after an image causes a Safari crash

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Deleting a Chinese character typed using pinyin on Mac on the same line as an image causes Safari to crash.

[Ryosuke Niwa]

<rdar://problem/27951933>

[Ryosuke Niwa]

Created attachment 297066 [details]
Fixes the crash

[Wenson Hsieh]

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

[Alexey Proskuryakov]

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?

[Ryosuke Niwa]

(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.

[Tim Horton]

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).

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 297160 [details]
Updated per review comments

[Alexey Proskuryakov]

> 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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 297380 [details]
Added a API test

[Ryosuke Niwa]

Created attachment 297381 [details]
Added a API test

[Wenson Hsieh]

Comment on attachment 297381 [details]
Added a API test

Test looks good to me!

[Ryosuke Niwa]

Created attachment 297385 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 297385 [details]
Patch for landing

Clearing flags on attachment: 297385

Committed r209957: <http://trac.webkit.org/changeset/209957>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.