Bug 31363 – fast/css/color-correction tests crash on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Brian Weinstein]

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.

[Brian Weinstein]

Created attachment 42982 [details]
Null Checks

[Beth Dakin]

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.

[Darin Adler]

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.

[Brian Weinstein]

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.

[Beth Dakin]

kCGColorSpaceSRGB should be available according to the header. Looks like we need to file a CG bug.

[Brian Weinstein]

Until then I will skip these tests on Windows.

[Brian Weinstein]

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.

[Darin Adler]

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

[Brian Weinstein]

Comment on attachment 42996 [details]
Add tests to Skipped List

Skipped list changes landed in r50841.

[Brian Weinstein]

Created attachment 43006 [details]
WebCore Workaround + Skipped List Removal

[Beth Dakin]

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.

[Darin Adler]

defined, not DEFINED

[Brian Weinstein]

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.

[Brian Weinstein]

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.

[Darin Adler]

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

[Brian Weinstein]

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

[Beth Dakin]

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

[Stephen White]

Committed r54502: <http://trac.webkit.org/changeset/54502>

[Stephen White]

Hmm, sorry about that.

Seems like "webkit-patch land" picked up the wrong bug ID; ignore the previous comment.

[Brent Fulgham]

The NULL color space issue is now corrected.