RESOLVED FIXED139969
[WinCairo] Crash when font data pointer is null. 
https://bugs.webkit.org/show_bug.cgi?id=139969
Summary [WinCairo] Crash when font data pointer is null.
peavo
Reported 2014-12-27 13:01:48 PST
There is missing a null pointer check in the UniscribeController::shape() method. I have gotten several crashes here because the font data pointer was null.
Attachments
Patch (1.37 KB, patch)
2014-12-27 13:07 PST, peavo 		no flags
DetailsFormatted DiffDiff
Patch (4.08 KB, patch)
2015-01-03 08:21 PST, peavo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
peavo
Comment 1 2014-12-27 13:07:49 PST
Created attachment 243779 [details] Patch
Sam Weinig
Comment 2 2014-12-27 13:57:16 PST
Comment on attachment 243779 [details] Patch Test case?
Csaba Osztrogonác
Comment 3 2014-12-27 14:39:32 PST
(In reply to comment #2) > Comment on attachment 243779 [details] > Patch > > Test case? test for wincairo? :) It's impossible to run any layout test on wincairo. (N)RWT doesn't know wincairo port (bug124927) and there isn't ORWT long time ago ...
Sam Weinig
Comment 4 2014-12-27 15:14:59 PST
(In reply to comment #3) > (In reply to comment #2) > > Comment on attachment 243779 [details] > > Patch > > > > Test case? > > test for wincairo? :) > > It's impossible to run any layout test on wincairo. (N)RWT doesn't > know wincairo port (bug124927) and there isn't ORWT long time ago ... So, is it time to remove support for WinCairo? A port without any testing seems like a bad idea. Do you at least have a test case that crashes in a WinCairo mini browser?
Alex Christensen
Comment 5 2014-12-28 00:05:21 PST
I think we should not need both an assert and a null check. Asserts should be for things that should never be false. Sam, I have hardware for a WinCairo bot. See https://bugs.webkit.org/show_bug.cgi?id=139908 The few differences between AppleWin and WinCairo are tested manually and that has worked well for some time now.
Alex Christensen
Comment 6 2014-12-28 00:32:21 PST
And layout tests can be run with WinCairo, it just requires a bit of impersonating AppleWin right now.
peavo
Comment 7 2014-12-28 12:31:36 PST
(In reply to comment #2) > Comment on attachment 243779 [details] > Patch > > Test case? I will try to come up with a test case :)
peavo
Comment 8 2014-12-29 13:32:27 PST
Here is a somewhat reduced test case which crashes the browser every time: <html> <head> <style> span.texhtml{font-family:"Nimbus Roman No9 L","Times New Roman",Times,serif;font-size:118%;line-height:1} .texhtml{-webkit-font-feature-settings:"lnum","tnum";font-feature-settings:"lnum","tnum"} </style> </head> <body> <span class="texhtml">|<span class="Unicode">⟩</span></span> </body> </html>
Alex Christensen
Comment 9 2014-12-29 19:43:41 PST
Comment on attachment 243779 [details] Patch I assume this change fixes it so it doesn't crash. Then add the test to layout tests and upload a new patch.
peavo
Comment 10 2015-01-03 08:21:33 PST
Created attachment 243917 [details] Patch
peavo
Comment 11 2015-01-03 08:22:59 PST
(In reply to comment #9) > Comment on attachment 243779 [details] > Patch > > I assume this change fixes it so it doesn't crash. Then add the test to > layout tests and upload a new patch. Thanks for reviewing :) Updated patch.
WebKit Commit Bot
Comment 12 2015-01-05 07:05:48 PST
Comment on attachment 243917 [details] Patch Clearing flags on attachment: 243917 Committed r177909: <http://trac.webkit.org/changeset/177909>
WebKit Commit Bot
Comment 13 2015-01-05 07:05:53 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.