Bug 106631 – [chromium] REGRESSION(r139347) roll chromium deps broke webkit-unit-tests

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

[noel gordon]

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

[noel gordon]

webkit-unit-tests also broke on chromium-win.

[noel gordon]

http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29?numbuilds=50

[noel gordon]

Created attachment 182264 [details]
Patch

[noel gordon]

Patch disables these tests, should we need to go there.

[Eric Seidel (no email)]

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?

[noel gordon]

> 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

[Eric Seidel (no email)]

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.

[noel gordon]

Passing on the linux bot.  Maybe I should add #ifdefs and disable MAC and WIN only.  Thoughts?

[Philip Rogers]

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

[noel gordon]

Created attachment 182269 [details]
Patch

[noel gordon]

Comment on attachment 182269 [details]
Patch

Clearing flags on attachment: 182269

Committed r139411: <http://trac.webkit.org/changeset/139411>

[noel gordon]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

Reopened to address renabling these tests on mac and win.

[Stephen Chenney]

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.

[Philip Rogers]

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

[noel gordon]

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

[Nico Weber]

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.

[noel gordon]

Indeed. Chromium-side fix https://codereview.chromium.org/12038003

[noel gordon]

Created attachment 183862 [details]
Patch

[WebKit Review Bot]

Comment on attachment 183862 [details]
Patch

Clearing flags on attachment: 183862

Committed r140379: <http://trac.webkit.org/changeset/140379>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.