Bug 47240

It's only happening on osx. Here's an example: 2010-10-06 09:23:34,593 executive.py:315 DEBUG "/b/slave/webkit-rel-mac-webkit-org/build/src/xcodebuild/Release/image_diff --diff /tmp/tmparACnM/expected.png /tmp/tmparACnM/actual.png /b/slave/webkit-rel-mac-webkit-org/build/src/webkit/Release/../../../layout-test-results/tables/mozilla_expected_failures/bugs/bug10140-diff.png" took 0.06s 2010-10-06 09:23:34,594 chromium.py:160 ERROR image diff returned an exit code of 2 This is the only test that causes the error. Maybe the checked in png is bad?

Created attachment 69994 [details] actual Here's the png that image_diff is failing to read. AFAICT, this is a valid png but we never get the end callback when decoding it.

(In reply to comment #2) > Created an attachment (id=69994) [details] > actual > > Here's the png that image_diff is failing to read. AFAICT, this is a valid png but we never get the end callback when decoding it. But it works fine for me when I copy the image to my Linux machine and feed it to image diff. Maybe there's some OSX specific code causing libpng decode to fail for this file?

hm. i thought someone (shans@?) saw this on windows as well ... regardless of whether not there's something wrong with this PNG, it's clear that we hadn't really considered what would happen if image_diff/ImageDiff returns an error. I'm not sure what the right thing to do is, but I would propose that it's something along the lines of (a) log a useful error message at the ERROR level so that it actually shows up all the time, and (b) mark the test as failed, but continue on. By "useful error message", I mean something like we have in the above log, but actually return the full command that failed (like we do in the execute debug statement, but at the ERROR level).

Taking ownership to see if this is still an issue and if we need to do anything more about it.

I don't think it should block bug 34984 and I plan to clear the blocking flag on Mon 4/11. If you disagree, now would be a good time to speak up :).

clearing blocking flag.

disclaiming ownership on this as I'm probably not going to be working on it any time soon. Adding Eric and Adam in case they are interested.

I'm able to reproduce this problem on windows. Furthermore, it seems like I can switch the args of the ImageDiff command, and it always fails on the first png being given to it. So its unlikely to do with the validity of the png itself. I'll keep digging into it, but only for a short while. If I can solve it, I'll take ownership of this bug.

OK, isolated the problem: the new-run-webkit-tests version of /tmp refers to my cygwin environment temp. the ImageDiff /tmp refers to the windows environment, C:\tmp. So, if people can suggest answers for the following, I can try to resolve it: - is there a way to mark that new-run-webkit-tests failed due to internal error, rather than test failure? That seems like the most ideal choice. the ERROR message is there now, but is easily overlooked when the test passes, and failing the test would be badly misleading. - what is the "right" way to solve it? option 1: if there is a way to detect cygwin environment in python, we can export a CYGWIN_ROOT path, and if that environment exists, ImageDiff can prepend absolute paths option 2: somehow use relative paths to ImageDiff ?

ORWT lauches the ImageDiff tool and feeds it Images via stdin/stdout. I think NRWT does that for normal WebKit ports too. TestShell/Chromium has always written the image files out to disk (and I assume it launches a new copy of ImageDiff for each diff it produces.

(In reply to comment #10) > OK, isolated the problem: > > the new-run-webkit-tests version of /tmp refers to my cygwin environment temp. the ImageDiff /tmp refers to the windows environment, C:\tmp. > It sounds like we simply need to wrap the paths we are passing to ImageDiff in calls to cygpath() to get them converted to the native win32 paths. Try changing ChromiumPort.diff_image() in chromium.py to something like the following (trying from memory; don't have a win machine in front of me to test): tempdir = self._filesystem.mkdtemp() expected_filename = self._filesystem.join(str(tempdir), "expected.png") self._filesystem.write_binary_file(expected_filename, expected_contents) actual_filename = self._filesystem.join(str(tempdir), "actual.png") self._filesystem.write_binary_file(actual_filename, actual_contents) if sys.platform == 'cygwin': native_expected_filename = path.cygpath(expected_filename) native_actual_filename = path.cygpath(actual_filename) if diff_filename: native_diff_filename = path.cygpath(diff_filename) else: native_expected_filename = expected_filename native_actual_filename = actual_filename if diff_filename: native_diff_filename = diff_filename executable = self._path_to_image_diff() if diff_filename: cmd = [executable, '--diff', native_expected_filename, native_actual_filename, native_diff_filename] else: cmd = [executable, native_expected_filename, native_actual_filename] If this doesn't work, I can track down the right incantation necessary tomorrow. We shouldn't have to do the more complicated things you're suggesting since cygpath() can handle the conversions for us.

Created attachment 104374 [details] Patch

Thanks for hunting this down!

Attachment 104374 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 104375 [details] Patch

Comment on attachment 104375 [details] Patch Clearing flags on attachment: 104375 Committed r93345: <http://trac.webkit.org/changeset/93345>

All reviewed patches have been landed. Closing bug.

Does this patch fix the original problem mentioned in the bug? Namely that running ImageDiff on mac against the attached png returns an error code of 2.

For windows/cygwin, it should. There could be several other reasons ImageDiff returns and exit status of 2, so the original problem especially on osx may have had a different cause as well. If you think its best to change the state of this bug, that makes sense to me.

Still likely a problem on OSX.

tony, to be clear, are you looking for NRWT to do something different if there is an error returned, or are you looking to see why an error is being returned in this case (or both)?

(In reply to comment #22) > tony, to be clear, are you looking for NRWT to do something different if there is an error returned, or are you looking to see why an error is being returned in this case (or both)? The latter. Seems like a bug in image_diff.

This patch appears to have broken a python test on Windows 7 bots: http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/15620 http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/15621

OK, I checked... it seems like test_diff_image in chromium_unittest.py needs updating. I'll submit another patch after I verify the most appropriate way to fix it.

FYI, I submitted the NRWT windows/cygwin fix in another bug: https://bugs.webkit.org/show_bug.cgi?id=67078

Curious what is the status here? Should this still be open? Should it block bug 34984 or bug 64491?

I think the status is that its unclear how to reproduce it on osx. Personally I have never encountered the problem on osx. However, its not my place to say that we can close the bug. If it stays open, I think it is worth putting on the blocks list for those issues you mentioned.

Dirk? Is there more to do here? Just trying to clean up the list of bugs so I can get a sense for how much NRWT work there is left to do. :)

(In reply to comment #29) > Dirk? Is there more to do here? Just trying to clean up the list of bugs so I can get a sense for how much NRWT work there is left to do. :) Yes, the original issue (also described by tony in comment #11) is still unanswered.

I've just posted a patch in bug 92934 that'll actually cause this to be an image diff failure, so if this is still happening we'll likely have to treat it seriously.

Chromium specific.