Bug 31363

Summary: fast/css/color-correction tests crash on Windows
Product: WebKit Reporter: Brian Weinstein <bweinstein>
Component: Tools / TestsAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, bdakin, bfulgham, darin, mitz, senorblanco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Attachments:
Description Flags
Null Checks
darin: review-, bweinstein: commit-queue-
Add tests to Skipped List
none
WebCore Workaround + Skipped List Removal none

Description Brian Weinstein 2009-11-11 11:12:03 PST
This is caused by sRGBColorSpaceRef() returning null, which causes CFColorCreate to return null, and then when CFRelease(cgColor) is called, that causes a crash.

This is all in WebCore/platform/graphics/cg/GraphicsContextCG.cpp.
Comment 1 Brian Weinstein 2009-11-11 11:19:57 PST
Created attachment 42982 [details]
Null Checks
Comment 2 Beth Dakin 2009-11-11 12:40:53 PST
I wonder why this is failing. Brian, do you know if it ALWAYS fails? We need to find out kCGColorSpaceSRGB is supposed to be available on Windows. If it is not available and it always fails, that is a bummer because it means this feature won't work on Windows. 

If this feature really doesn't exist on Windows, then the best solution is probably to edit the #ifdef in sRGBColorSpace() and make it return deviceColorSpace for Tiger and Windows. Then we will not need the null checks, and we will still draw something when the property is specified.
Comment 3 Darin Adler 2009-11-11 12:44:28 PST
Comment on attachment 42982 [details]
Null Checks

I don’t understand why this is appropriate. Is it really OK to just do the wrong thing and use the wrong color? Doesn't this break the feature on Windows and worse, make it use the wrong color in various places?

> -        CGContextSetStrokeColorSpace(context, sRGBColorSpaceRef());
> +        if (CGColorSpaceRef sRGBColorSpace = sRGBColorSpaceRef())
> +            CGContextSetFillColorSpace(context, sRGBColorSpace);

You changed stroke to fill here.

If Windows CG is getting 0 for some of these things we need to figure out why. Just doing the wrong thing to avoid a crash is not good. In the mean time, skipping the tests might be the best short term measure.

review- because of the stroke to fill change and also my general disagreement that this is a fix.
Comment 4 Brian Weinstein 2009-11-11 12:47:13 PST
Understood, this patch was to get feedback on the best way to handle it, which I now have plenty of. I like Beth's solution, and think that would be the right way to handle it for now.
Comment 5 Beth Dakin 2009-11-11 12:50:14 PST
kCGColorSpaceSRGB should be available according to the header. Looks like we need to file a CG bug.
Comment 6 Brian Weinstein 2009-11-11 13:34:11 PST
Until then I will skip these tests on Windows.
Comment 7 Brian Weinstein 2009-11-11 13:52:17 PST
Created attachment 42996 [details]
Add tests to Skipped List

Should we also add a temporary check in 

static CGColorSpaceRef sRGBColorSpaceRef() to return deviceRGBColorSpaceRef() if we are on Windows until the bug is fixed? If so, I can do that in another patch.
Comment 8 Darin Adler 2009-11-11 13:53:34 PST
(In reply to comment #7)
> Should we also add a temporary check in 
> 
> static CGColorSpaceRef sRGBColorSpaceRef() to return deviceRGBColorSpaceRef()
> if we are on Windows until the bug is fixed? If so, I can do that in another
> patch.

You should talk to Beth and Adele about that.
Comment 9 Brian Weinstein 2009-11-11 13:58:54 PST
Comment on attachment 42996 [details]
Add tests to Skipped List

Skipped list changes landed in r50841.
Comment 10 Brian Weinstein 2009-11-11 14:38:36 PST
Created attachment 43006 [details]
WebCore Workaround + Skipped List Removal
Comment 11 Beth Dakin 2009-11-11 14:42:25 PST
Comment on attachment 43006 [details]
WebCore Workaround + Skipped List Removal

> +#if PLATFORM(WIN)
> +    return deviceRGBColorSpaceRef();
> +#else
>  #ifdef BUILDING_ON_TIGER
>      return deviceRGBColorSpaceRef();
>  #else
>      static CGColorSpaceRef sRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB);
>      return sRGBSpace;
>  #endif
> +#endif
>  }

I think you can re-write this as:

#if PLATORM(WIN) || DEFINED(BUILDING_ON_TIGER)
   return deviceRGBColorSpaceRef();
#else
      static CGColorSpaceRef sRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB);
      return sRGBSpace;
#endif

which might be a little neater, but it's up to you.
Comment 12 Darin Adler 2009-11-11 14:43:58 PST
defined, not DEFINED
Comment 13 Brian Weinstein 2009-11-11 14:48:27 PST
Happy I re-compiled after making that fix. That is a lot cleaner, I am making that change and landing. Keeping the bug open though until this workaround is fixed. I'll add a FIXME as well referencing this bug.
Comment 14 Brian Weinstein 2009-11-11 14:50:50 PST
Comment on attachment 43006 [details]
WebCore Workaround + Skipped List Removal

Landed in r50843. Keeping the bug open until we can use correct sCGColorSpaceSRGB without crashing.
Comment 15 Darin Adler 2009-11-11 14:52:34 PST
(In reply to comment #14)
> Keeping the bug open until we can use correct sCGColorSpaceSRGB without crashing.

I'm not sure that's right. Lets make a new bug talking about what's really wrong.
Comment 16 Brian Weinstein 2009-11-11 14:55:18 PST
(In reply to comment #15)
> (In reply to comment #14)
> > Keeping the bug open until we can use correct sCGColorSpaceSRGB without crashing.
> 
> I'm not sure that's right. Lets make a new bug talking about what's really
> wrong.

Sure, that's fair. I'll close this, since the tests aren't crashing anymore.
Comment 17 Beth Dakin 2009-11-11 20:34:10 PST
<https://bugs.webkit.org/show_bug.cgi?id=31316>

and

<rdar://problem/7387791> Make -webkit-color-correction correct color on Windows

To track re-enabling this feature. I also filed a CG bug linked to from the Radar.
Comment 18 Stephen White 2010-02-08 12:26:04 PST
Committed r54502: <http://trac.webkit.org/changeset/54502>
Comment 19 Stephen White 2010-02-08 12:32:58 PST
Hmm, sorry about that.

Seems like "webkit-patch land" picked up the wrong bug ID; ignore the previous comment.
Comment 20 Brent Fulgham 2013-11-22 13:23:59 PST
The NULL color space issue is now corrected.