31363 2009-11-11 11:12:03 -0800 fast/css/color-correction tests crash on Windows 2013-11-22 13:25:58 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC Windows 7 RESOLVED FIXED P2 Normal --- 1 bweinstein bfulgham aroben bdakin bfulgham darin mitz senorblanco oldest_to_newest 162572 0 bweinstein 2009-11-11 11:12:03 -0800 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. 162582 1 42982 bweinstein 2009-11-11 11:19:57 -0800 Created attachment 42982 Null Checks 162619 2 bdakin 2009-11-11 12:40:53 -0800 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. 162620 3 42982 darin 2009-11-11 12:44:28 -0800 Comment on attachment 42982 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. 162623 4 bweinstein 2009-11-11 12:47:13 -0800 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. 162626 5 bdakin 2009-11-11 12:50:14 -0800 kCGColorSpaceSRGB should be available according to the header. Looks like we need to file a CG bug. 162655 6 bweinstein 2009-11-11 13:34:11 -0800 Until then I will skip these tests on Windows. 162660 7 42996 bweinstein 2009-11-11 13:52:17 -0800 Created attachment 42996 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. 162662 8 darin 2009-11-11 13:53:34 -0800 (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. 162667 9 42996 bweinstein 2009-11-11 13:58:54 -0800 Comment on attachment 42996 Add tests to Skipped List Skipped list changes landed in r50841. 162684 10 43006 bweinstein 2009-11-11 14:38:36 -0800 Created attachment 43006 WebCore Workaround + Skipped List Removal 162687 11 43006 bdakin 2009-11-11 14:42:25 -0800 Comment on attachment 43006 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. 162689 12 darin 2009-11-11 14:43:58 -0800 defined, not DEFINED 162693 13 bweinstein 2009-11-11 14:48:27 -0800 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. 162695 14 43006 bweinstein 2009-11-11 14:50:50 -0800 Comment on attachment 43006 WebCore Workaround + Skipped List Removal Landed in r50843. Keeping the bug open until we can use correct sCGColorSpaceSRGB without crashing. 162696 15 darin 2009-11-11 14:52:34 -0800 (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. 162698 16 bweinstein 2009-11-11 14:55:18 -0800 (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. 162784 17 bdakin 2009-11-11 20:34:10 -0800 <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. 188720 18 senorblanco 2010-02-08 12:26:04 -0800 Committed r54502: <http://trac.webkit.org/changeset/54502> 188723 19 senorblanco 2010-02-08 12:32:58 -0800 Hmm, sorry about that. Seems like "webkit-patch land" picked up the wrong bug ID; ignore the previous comment. 953664 20 bfulgham 2013-11-22 13:23:59 -0800 The NULL color space issue is now corrected. 42982 2009-11-11 11:19:57 -0800 2009-11-11 12:47:49 -0800 Null Checks color_nullcheck.patch text/plain 2354 bweinstein SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1MDgyNSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTggQEAKKzIwMDktMTEtMTEgIEJyaWFuIFdlaW5zdGVpbiAgPGJ3ZWluc3RlaW5A YXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIEFkZCBudWxsIGNoZWNrcyBmb3IgQ0dDb2xvclJlZnMgYW5kIENHQ29sb3JTcGFjZVJlZnMg dG8gcHJldmVudCBjcmFzaGVzCisgICAgICAgIHdoZW4gdGhleSByZXR1cm4gbnVsbC4KKworICAg ICAgICBUaGlzIGZpeGVzIHRoZSBmYXN0L2Nzcy9jb2xvci1jb3JyZWN0aW9uIHRlc3RzIG9uIFdp bmRvd3MuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9jZy9HcmFwaGljc0NvbnRleHRD Ry5jcHA6CisgICAgICAgIChXZWJDb3JlOjpzZXRDR0ZpbGxDb2xvcik6CisgICAgICAgIChXZWJD b3JlOjpzZXRDR1N0cm9rZUNvbG9yKToKKyAgICAgICAgKFdlYkNvcmU6OnNldENHRmlsbENvbG9y U3BhY2UpOgorICAgICAgICAoV2ViQ29yZTo6c2V0Q0dTdHJva2VDb2xvclNwYWNlKToKKwogMjAw OS0xMS0xMSAgTmF0ZSBDaGFwaW4gIDxqYXBoZXRAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJl dmlld2VkIGJ5IERpbWl0cmkgR2xhemtvdi4KSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhp Y3MvY2cvR3JhcGhpY3NDb250ZXh0Q0cuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvcGxhdGZv cm0vZ3JhcGhpY3MvY2cvR3JhcGhpY3NDb250ZXh0Q0cuY3BwCShyZXZpc2lvbiA1MDgxNikKKysr IFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2cvR3JhcGhpY3NDb250ZXh0Q0cuY3BwCSh3b3Jr aW5nIGNvcHkpCkBAIC04OCw2ICs4OCw5IEBAIHN0YXRpYyB2b2lkIHNldENHRmlsbENvbG9yKENH Q29udGV4dFJlZiAKICAgICBlbHNlCiAgICAgICAgIGNnQ29sb3IgPSBDR0NvbG9yQ3JlYXRlKGRl dmljZVJHQkNvbG9yU3BhY2VSZWYoKSwgY29tcG9uZW50cyk7CiAKKyAgICBpZiAoIWNnQ29sb3Ip CisgICAgICAgIHJldHVybjsKKwogICAgIENHQ29udGV4dFNldEZpbGxDb2xvcldpdGhDb2xvcihj b250ZXh0LCBjZ0NvbG9yKTsKICAgICBDRlJlbGVhc2UoY2dDb2xvcik7CiB9CkBAIC0xMDMsNiAr MTA2LDkgQEAgc3RhdGljIHZvaWQgc2V0Q0dTdHJva2VDb2xvcihDR0NvbnRleHRSZQogICAgIGVs c2UKICAgICAgICAgY2dDb2xvciA9IENHQ29sb3JDcmVhdGUoZGV2aWNlUkdCQ29sb3JTcGFjZVJl ZigpLCBjb21wb25lbnRzKTsKIAorICAgIGlmICghY2dDb2xvcikKKyAgICAgICAgcmV0dXJuOwor CiAgICAgQ0dDb250ZXh0U2V0U3Ryb2tlQ29sb3JXaXRoQ29sb3IoY29udGV4dCwgY2dDb2xvcik7 CiAgICAgQ0ZSZWxlYXNlKGNnQ29sb3IpOwogfQpAQCAtMTEzLDcgKzExOSw4IEBAIHN0YXRpYyB2 b2lkIHNldENHRmlsbENvbG9yU3BhY2UoQ0dDb250ZXgKICAgICBjYXNlIERldmljZUNvbG9yU3Bh Y2U6CiAgICAgICAgIGJyZWFrOwogICAgIGNhc2Ugc1JHQkNvbG9yU3BhY2U6Ci0gICAgICAgIENH Q29udGV4dFNldEZpbGxDb2xvclNwYWNlKGNvbnRleHQsIHNSR0JDb2xvclNwYWNlUmVmKCkpOwor ICAgICAgICBpZiAoQ0dDb2xvclNwYWNlUmVmIHNSR0JDb2xvclNwYWNlID0gc1JHQkNvbG9yU3Bh Y2VSZWYoKSkKKyAgICAgICAgICAgIENHQ29udGV4dFNldEZpbGxDb2xvclNwYWNlKGNvbnRleHQs IHNSR0JDb2xvclNwYWNlKTsKICAgICAgICAgYnJlYWs7CiAgICAgZGVmYXVsdDoKICAgICAgICAg QVNTRVJUX05PVF9SRUFDSEVEKCk7CkBAIC0xMjcsNyArMTM0LDggQEAgc3RhdGljIHZvaWQgc2V0 Q0dTdHJva2VDb2xvclNwYWNlKENHQ29udAogICAgIGNhc2UgRGV2aWNlQ29sb3JTcGFjZToKICAg ICAgICAgYnJlYWs7CiAgICAgY2FzZSBzUkdCQ29sb3JTcGFjZToKLSAgICAgICAgQ0dDb250ZXh0 U2V0U3Ryb2tlQ29sb3JTcGFjZShjb250ZXh0LCBzUkdCQ29sb3JTcGFjZVJlZigpKTsKKyAgICAg ICAgaWYgKENHQ29sb3JTcGFjZVJlZiBzUkdCQ29sb3JTcGFjZSA9IHNSR0JDb2xvclNwYWNlUmVm KCkpCisgICAgICAgICAgICBDR0NvbnRleHRTZXRGaWxsQ29sb3JTcGFjZShjb250ZXh0LCBzUkdC Q29sb3JTcGFjZSk7CiAgICAgICAgIGJyZWFrOwogICAgIGRlZmF1bHQ6CiAgICAgICAgIEFTU0VS VF9OT1RfUkVBQ0hFRCgpOwo= 42996 2009-11-11 13:52:17 -0800 2009-11-11 14:38:50 -0800 Add tests to Skipped List skip_color_tests.patch text/plain 1092 bweinstein SW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9D aGFuZ2VMb2cJKHJldmlzaW9uIDUwODQwKQorKysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3Jr aW5nIGNvcHkpCkBAIC0xLDMgKzEsMTIgQEAKKzIwMDktMTEtMTEgIEJyaWFuIFdlaW5zdGVpbiAg PGJ3ZWluc3RlaW5AYXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFNraXAgdGhlIHdlYmtpdC1jb2xvci1jb3JyZWN0aW9uIHRlc3RzIGJl Y2F1c2UgdGhleSBjcmFzaCBvbiBXaW5kb3dzIAorICAgICAgICBkdWUgdG8gYSBDb3JlR3JhcGhp Y3MgYnVnLgorCisgICAgICAgICogcGxhdGZvcm0vd2luL1NraXBwZWQ6CisKIDIwMDktMTEtMTEg IEVyaWMgWi4gQXllcnMgIDx6dW5kZWxAZ29vZ2xlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBi eSBQYXZlbCBGZWxkbWFuLgpJbmRleDogTGF5b3V0VGVzdHMvcGxhdGZvcm0vd2luL1NraXBwZWQK PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gTGF5b3V0VGVzdHMvcGxhdGZvcm0vd2luL1NraXBwZWQJKHJldmlzaW9u IDUwODE2KQorKysgTGF5b3V0VGVzdHMvcGxhdGZvcm0vd2luL1NraXBwZWQJKHdvcmtpbmcgY29w eSkKQEAgLTY4MiwzICs2ODIsOSBAQCBmYXN0L2RvbS9IVE1MT2JqZWN0RWxlbWVudC9jaGlsZHJl bi1jaGFuCiAKICMgTWlzc2luZyBTb2NrZXRTdHJlYW1IYW5kbGUgaW1wbGVtZW50YXRpb24KIHdl YnNvY2tldC90ZXN0cworCisjIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0zMTM2MworZmFzdC9jc3MvY29sb3ItY29ycmVjdGlvbi1iYWNrZ3JvdW5kcy1hbmQtdGV4dC5o dG1sCitmYXN0L2Nzcy9jb2xvci1jb3JyZWN0aW9uLW9uLWJhY2tncm91bmRzLmh0bWwKK2Zhc3Qv Y3NzL2NvbG9yLWNvcnJlY3Rpb24tb24tdGV4dC5odG1sCitmYXN0L2Nzcy9jb2xvci1jb3JyZWN0 aW9uLmh0bWwK 43006 2009-11-11 14:38:36 -0800 2009-11-11 14:50:50 -0800 WebCore Workaround + Skipped List Removal unskip_colors.patch text/plain 2453 bweinstein SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1MDg0MikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTMgQEAKKzIwMDktMTEtMTEgIEJyaWFuIFdlaW5zdGVpbiAgPGJ3ZWluc3RlaW5A YXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIElmIHdlIGFyZSBvbiBXaW5kb3dzLCBkb24ndCB0cnkgYW5kIGdldCB0aGUga0NHQ29sb3JT cGFjZVNSR0IgQ29sb3JTcGFjZSwKKyAgICAgICAgYmVjYXVzZSB0aGVyZSBpcyBhIENHIGJ1ZyBw cmV2ZW50aW5nIHRoaXMgZnJvbSB3b3JraW5nLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhp Y3MvY2cvR3JhcGhpY3NDb250ZXh0Q0cuY3BwOgorICAgICAgICAoV2ViQ29yZTo6c1JHQkNvbG9y U3BhY2VSZWYpOgorCiAyMDA5LTExLTExICBQYXZlbCBGZWxkbWFuICA8cGZlbGRtYW5AY2hyb21p dW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IFRpbW90aHkgSGF0Y2hlci4KSW5kZXg6IFdl YkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2cvR3JhcGhpY3NDb250ZXh0Q0cuY3BwCj09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT0KLS0tIFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2cvR3JhcGhpY3NDb250ZXh0Q0cuY3Bw CShyZXZpc2lvbiA1MDgxNikKKysrIFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2cvR3JhcGhp Y3NDb250ZXh0Q0cuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02OSwxMiArNjksMTYgQEAgc3RhdGlj IENHQ29sb3JTcGFjZVJlZiBkZXZpY2VSR0JDb2xvclNwYQogCiBzdGF0aWMgQ0dDb2xvclNwYWNl UmVmIHNSR0JDb2xvclNwYWNlUmVmKCkKIHsKKyNpZiBQTEFURk9STShXSU4pCisgICAgcmV0dXJu IGRldmljZVJHQkNvbG9yU3BhY2VSZWYoKTsKKyNlbHNlCiAjaWZkZWYgQlVJTERJTkdfT05fVElH RVIKICAgICByZXR1cm4gZGV2aWNlUkdCQ29sb3JTcGFjZVJlZigpOwogI2Vsc2UKICAgICBzdGF0 aWMgQ0dDb2xvclNwYWNlUmVmIHNSR0JTcGFjZSA9IENHQ29sb3JTcGFjZUNyZWF0ZVdpdGhOYW1l KGtDR0NvbG9yU3BhY2VTUkdCKTsKICAgICByZXR1cm4gc1JHQlNwYWNlOwogI2VuZGlmCisjZW5k aWYKIH0KIAogc3RhdGljIHZvaWQgc2V0Q0dGaWxsQ29sb3IoQ0dDb250ZXh0UmVmIGNvbnRleHQs IGNvbnN0IENvbG9yJiBjb2xvciwgQ29sb3JTcGFjZSBjb2xvclNwYWNlKQpJbmRleDogTGF5b3V0 VGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL0NoYW5nZUxvZwkocmV2 aXNpb24gNTA4NDIpCisrKyBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAg LTEsMyArMSwxMiBAQAorMjAwOS0xMS0xMSAgQnJpYW4gV2VpbnN0ZWluICA8YndlaW5zdGVpbkBh cHBsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgVW5za2lwIHRoZSB3ZWJraXQtY29sb3ItY29ycmVjdGlvbiB0ZXN0cyBiZWNhdXNlIHRoZSB3 b3JrYXJvdW5kIG9mIHVzaW5nCisgICAgICAgIHRoZSBkZXZpY2VSR0JDb2xvclNwYWNlIGFsbG93 cyB0aGUgdGVzdHMgdG8gcGFzcyBvbiBXaW5kb3dzLgorCisgICAgICAgICogcGxhdGZvcm0vd2lu L1NraXBwZWQ6CisKIDIwMDktMTEtMTEgIEJyaWFuIFdlaW5zdGVpbiAgPGJ3ZWluc3RlaW5AYXBw bGUuY29tPgogCkluZGV4OiBMYXlvdXRUZXN0cy9wbGF0Zm9ybS93aW4vU2tpcHBlZAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBMYXlvdXRUZXN0cy9wbGF0Zm9ybS93aW4vU2tpcHBlZAkocmV2aXNpb24gNTA4NDEp CisrKyBMYXlvdXRUZXN0cy9wbGF0Zm9ybS93aW4vU2tpcHBlZAkod29ya2luZyBjb3B5KQpAQCAt NjgyLDkgKzY4MiwzIEBAIGZhc3QvZG9tL0hUTUxPYmplY3RFbGVtZW50L2NoaWxkcmVuLWNoYW4K IAogIyBNaXNzaW5nIFNvY2tldFN0cmVhbUhhbmRsZSBpbXBsZW1lbnRhdGlvbgogd2Vic29ja2V0 L3Rlc3RzCi0KLSMgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTMxMzYz Ci1mYXN0L2Nzcy9jb2xvci1jb3JyZWN0aW9uLWJhY2tncm91bmRzLWFuZC10ZXh0Lmh0bWwKLWZh c3QvY3NzL2NvbG9yLWNvcnJlY3Rpb24tb24tYmFja2dyb3VuZHMuaHRtbAotZmFzdC9jc3MvY29s b3ItY29ycmVjdGlvbi1vbi10ZXh0Lmh0bWwKLWZhc3QvY3NzL2NvbG9yLWNvcnJlY3Rpb24uaHRt bAo=