Bug 139969 - [WinCairo] Crash when font data pointer is null.
Summary: [WinCairo] Crash when font data pointer is null.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-27 13:01 PST by peavo
Modified: 2015-01-05 07:05 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.37 KB, patch)
2014-12-27 13:07 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (4.08 KB, patch)
2015-01-03 08:21 PST, peavo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description peavo 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.
Comment 1 peavo 2014-12-27 13:07:49 PST
Created attachment 243779 [details]
Patch
Comment 2 Sam Weinig 2014-12-27 13:57:16 PST
Comment on attachment 243779 [details]
Patch

Test case?
Comment 3 Csaba Osztrogonác 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 ...
Comment 4 Sam Weinig 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?
Comment 5 Alex Christensen 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.
Comment 6 Alex Christensen 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.
Comment 7 peavo 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 :)
Comment 8 peavo 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>
Comment 9 Alex Christensen 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.
Comment 10 peavo 2015-01-03 08:21:33 PST
Created attachment 243917 [details]
Patch
Comment 11 peavo 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.
Comment 12 WebKit Commit Bot 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>
Comment 13 WebKit Commit Bot 2015-01-05 07:05:53 PST
All reviewed patches have been landed.  Closing bug.