Created attachment 182260 [details] r139347-mac-chrome-webkit-unit-test-bustage http://trac.webkit.org/changeset/139347 rolled chromium deps to 176117. On landing, r139347 broke the webkit-unit-tests on chrome-mac chrome-mac: http://build.webkit.org/builders/Chromium%20Mac%20Release%20%28Tests%29?numbuilds=50
webkit-unit-tests also broke on chromium-win.
http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29?numbuilds=50
Created attachment 182264 [details] Patch
Patch disables these tests, should we need to go there.
Comment on attachment 182264 [details] Patch So we don't know what part of the roll caused the regression? I assume we've CC'd relevant folks? It would be a shame to leave these failing. How bad are these failures? Do we know what's broken?
> So we don't know what part of the roll caused the regression? Nope. > I assume we've CC'd relevant folks? Yes, they are cc-ed. Some part of a two-sided chromium skia.gyp flag change. > It would be a shame to leave these failing. Agree. I compiled the webkit unit test locally on mac and tried to reproduce. Failed, the unit-tests just crash. > How bad are these failures? Dunno. > Do we know what's broken? Nope. I have a sneaking suspicion it may be related to bug 105493 but as I said above, I can build these tests but I can't run them -- they crash
Comment on attachment 182264 [details] Patch Not a fan of disabling tests. But assuming you've notified the relevant folks, I guess this is our best course of action.
Passing on the linux bot. Maybe I should add #ifdefs and disable MAC and WIN only. Thoughts?
(In reply to comment #8) > Passing on the linux bot. Maybe I should add #ifdefs and disable MAC and WIN only. Thoughts? I think this is the best approach at the moment. I'm on Mac10.7.5 and the two tests pass at ToT, which makes this hard to debug. Noel isn't able to reproduce locally either. I think the Skia guys may have more context on this, which is why I recommend temporarily skipping (~6hrs).
Created attachment 182269 [details] Patch
Comment on attachment 182269 [details] Patch Clearing flags on attachment: 182269 Committed r139411: <http://trac.webkit.org/changeset/139411>
All reviewed patches have been landed. Closing bug.
Reopened to address renabling these tests on mac and win.
The Skia flag changes should have been neutral, in that they were removed from the chromium side and added to the WebKit side, with no net change. That makes me think the problem is elsewhere. In any event, I am going to remove all of those flags and rebaseline now. If it was a flag issue then we will need to fix it anyway.
(In reply to comment #14) > The Skia flag changes should have been neutral, in that they were removed from the chromium side and added to the WebKit side, with no net change. That makes me think the problem is elsewhere. > > In any event, I am going to remove all of those flags and rebaseline now. If it was a flag issue then we will need to fix it anyway. Not knowing the context around the flags, it seems like one flag was left off on the WebKit side: SK_IGNORE_QUAD_STROKE_FIX If this wasn't a skia roll, this bug needs more looking into. There were over 2000revs in the roll.
Caused by http://crrev.com/175642 In WebImageTest.cpp we have: static PassRefPtr<SharedBuffer> readFile(const char* fileName) { String filePath = webkit_support::GetWebKitRootDir(); filePath.append("/Source/WebKit/chromium/tests/data/"); filePath.append(fileName); long long fileSize; if (!getFileSize(filePath, fileSize)) return 0; PlatformFileHandle handle = openFile(filePath, OpenForRead); // FAILS right here on the webkit win mac bots: the handle is invalid. Synopsis: GetWebKitRootDir() calls GetWebKitRootDirFilePath() in https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/support/webkit_support.cc&q=GetWebKitRootDir%20s%20file:%5Esrc/&type=cs&l=274 which returns a path containing "../.." on win and mac bots. We can read the length (and other metadata) of a file with such a path via WebFileUtilitiesImpl::getFileInfo(path) https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webfileutilities_impl.cc&exact_package=chromium&q=webfileutilities_impl&type=cs&l=44 hence getFileSize() works. But due to http://crrev.com/175642, we are prevented from opening the file https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webfileutilities_impl.cc&exact_package=chromium&q=webfileutilities_impl&type=cs&l=95 because that uses base::CreatePlatformFile() which no longer opens paths containing ".." after http://crrev.com/175642
I think the fix is to convert the paths to an absolute path first. See https://codereview.chromium.org/11817018 and https://codereview.chromium.org/11829014 for an example.
Indeed. Chromium-side fix https://codereview.chromium.org/12038003
Created attachment 183862 [details] Patch
Comment on attachment 183862 [details] Patch Clearing flags on attachment: 183862 Committed r140379: <http://trac.webkit.org/changeset/140379>