Bug 135551 – [iOS] run-webkit-tests chokes on unterminated UTF-8 when writing a test result

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Farler]

When writing a crash log or some other output file, webkitpy.common.system.filesystem.write_text_file:227 is the crash point. The crash is reproducible, so I’ll see what about the output is messing this up. For some reason, it’s trying to decode UTF-8 as ascii:

UnicodeDecodeError(''ascii' codec can't decode byte 0xe2 in position 305: ordinal not in range(128)’) - characters vary.

[David Farler]

After fixing the filesystem.py code to explicitly encode the output as UTF-8, it looks like the offending string is something along the lines of:

2014-08-03 15:08:35.863 LayoutTestRelay[3655:180772] Couldn't launch iOS Simulator from /Applications/Xcode.app/Contents/Developer
untime/Applications/iOS Simulator.app: Error Domain=NSCocoaErrorDomain Code=4 "The application “iOS Simulator.app” could not be launched because it was not found." UserInfo=0x7f97aad15420 {NSURL=file:///Applications/Xcode.app/Contents/Developer%0Auntime/Applications/iOS%20Simulator.app, NSLocalizedDescription=The application “iOS Simulator.app” could not be launched because it was not found., NSUnderlyingError=0x7f97aad0c3c0 "The operation couldn’t be completed. (OSStatus error -43.)"}

Looks like something might be racing to print to stderr. Sigh … this was working fine before the rewrite. The launch services message is also unexpected, since it’s able to find the app just fine many times before hitting this. It might be that constructing that app URL has some weird characters in it too.

[David Farler]

Wow. xcode-select —print-path is sending back from NSTask:

/Applications/Xcode.app/Contents/Developer \n \x07  — ASCII 7 — as in BEL. It even beeps my console when tailing the system log. No wonder -[NSString stringByTrimmingCharactersInSet:] isn’t working. Luckily, there is -[NSMutableCharacterSet formUnionWithCharacterSet] and +[NSCharacterSet controlCharacterSet] as well.

Going to remove a couple of other items here that can affect stderr behavior

- print CRASH: <process name> <pid> to stdout instead of stderr since nothing ever prints to stdout during a crash but may print to stderr.

- launch services’ stability of checking on the workspace isn’t 100% reliable, especially when multiple clients are asking about it, so bake in a 10-retry mechanism with random-5 second sleep. There is no external way to relieve the condition.

- Set up DEVELOPER_DIR in the environment once, inside webkitpy. Lots of parallel calls to xcode-select can fail.

- Don’t try to explicitly boot the device while the simulator app is launching, it already does it and can cause a failure to be printed to stderr.

[David Farler]

Created attachment 235965 [details]
Patch

[David Farler]

Another thing I noticed: filesystem.py's write_text_file assumes ASCII in stderr, and I've got a couple of places in LayoutTestRelay that may print -[NSError description], which helpfully puts beautiful smart quotes in the actual error string. So, we'll have to decode the contents as UTF-8 before writing (this should have been done anyway because the writing file handle is locked to UTF-8 encoding).

[David Farler]

Created attachment 236053 [details]
Patch

[Daniel Bates]

Comment on attachment 236053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236053&action=review

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:178
> +    sleep(2);

I suggest we add a comment here that explain why we need to wait 2 seconds as it isn't obvious given that we already waited for the device to boot above.

> Tools/Scripts/webkitpy/port/ios.py:369
> +        except ScriptError:
> +            return '/Applications/Xcode.app/Contents/Developer'

How did you come to the decision to hard code the path "/Applications/Xcode.app/Contents/Developer" when xcode-select errors out as opposed to having the script fail with an error about the Xcode installation on the machine? In particular, what circumstances do you envision xcode-select failing and it being reasonable to assume that Xcode is installed?

[David Farler]

(In reply to comment #6)
> (From update of attachment 236053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236053&action=review
> 
> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:178
> > +    sleep(2);
> 
> I suggest we add a comment here that explain why we need to wait 2 seconds as it isn't obvious given that we already waited for the device to boot above.

I’ve made it so!

> 
> > Tools/Scripts/webkitpy/port/ios.py:369
> > +        except ScriptError:
> > +            return '/Applications/Xcode.app/Contents/Developer'
> 
> How did you come to the decision to hard code the path "/Applications/Xcode.app/Contents/Developer" when xcode-select errors out as opposed to having the script fail with an error about the Xcode installation on the machine? In particular, what circumstances do you envision xcode-select failing and it being reasonable to assume that Xcode is installed?

This came out of a race with xcode-select (and I believe LaunchServices) but I only check for the Xcode directory once and it will fail appropriately, so I removed that code.

[David Farler]

OK, one last patch rev and I think we’ll have it (I’m testing it now).

The last two Unicode problems encountered were:
- a completely random UTF-16 encoded test file, and
- the fact that webkitpy expects ascii everywhere in stderr, while also expecting to be able to encode stderr as UTF-8 implicitly with codecs. NSError can randomly put curly quotes into stderr. We should really not be using the codecs library anymore and switch to io to make porting to Python 3 easier (and we should also be doing that to make encode/decode more straightforward).

If there are any other problems, let’s open a new bug. This one is getting old.

[David Farler]

Created attachment 237835 [details]
Patch

[WebKit Commit Bot]

Attachment 237835 [details] did not pass style-queue:


ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:97:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:97:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Farler]

(In reply to comment #10)
> Attachment 237835 [details] did not pass style-queue:
> 
> 
> ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:97:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
> ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:97:  Extra space before ( in function call  [whitespace/parens] [4]
> Total errors found: 2 in 5 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This is a false positive. I filed https://bugs.webkit.org/show_bug.cgi?id=136663

[Daniel Bates]

Comment on attachment 237835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237835&action=review

> Tools/ChangeLog:8
> +        Reduce cases where it's possible to print random error messages to stderr.

So, there are still code paths that can lead to random error messages (garbage data?) being written to the standard error stream?

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:95
> +        uint8_t bytes[[data length]];
> +        [data getBytes:bytes length:[data length]];

Notice that that buffer bytes is never used.

> Tools/Scripts/webkitpy/common/system/filesystem.py:227
> +        with codecs.open(path, 'w', 'utf-8') as f:

Is this change necessary? According to the Python 2.7.8 codec documentation, <https://docs.python.org/2/library/codecs.html>, "utf-8" is an alias of "utf_8" which is an alias of "utf8". Brent Fulgham confirmed this in bug 136546, comment 4.

For completeness the Python 3.4.1 codec documentation describes the same aliases: <https://docs.python.org/3/library/codecs.html>.

[David Farler]

(In reply to comment #12)
> (From update of attachment 237835 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237835&action=review
> 
> > Tools/ChangeLog:8
> > +        Reduce cases where it's possible to print random error messages to stderr.
> 
> So, there are still code paths that can lead to random error messages (garbage data?) being written to the standard error stream?

It looks like we have all of the bases covered, since part of the problem is just perfectly valid Unicode in stderr, so I’ve also made webkitpy tolerant of utf-8 in stderr, where before it only (incorrectly) expected ascii. I don’t see any more symptoms on my end. So, perhaps you wanted me to change this log message?

> 
> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:95
> > +        uint8_t bytes[[data length]];
> > +        [data getBytes:bytes length:[data length]];
> 
> Notice that that buffer bytes is never used.

Ah, I think this might be leftovers from the Swift translation. I can cut this out.

> 
> > Tools/Scripts/webkitpy/common/system/filesystem.py:227
> > +        with codecs.open(path, 'w', 'utf-8') as f:
> 
> Is this change necessary? According to the Python 2.7.8 codec documentation, <https://docs.python.org/2/library/codecs.html>, "utf-8" is an alias of "utf_8" which is an alias of "utf8". Brent Fulgham confirmed this in bug 136546, comment 4.
> 
> For completeness the Python 3.4.1 codec documentation describes the same aliases: <https://docs.python.org/3/library/codecs.html>.

It’s actually not strictly necessary, it came from habit while I was testing. Those aliases you mentioned are for a deprecated library. The default name for UTF-8 encoding in general is ‘utf-8’ and that’s what Python developers will be used to. But if you’d rather I put it back it before landing, let me know. In general, all usage of the codecs library should be replaced by “io” (pre-Python 3) or the built-in primitives without prefix in Python 3.

[Daniel Bates]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 237835 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=237835&action=review
> > 
> > > Tools/ChangeLog:8
> > > +        Reduce cases where it's possible to print random error messages to stderr.
> > 
> > So, there are still code paths that can lead to random error messages (garbage data?) being written to the standard error stream?
> 
> It looks like we have all of the bases covered, since part of the problem is just perfectly valid Unicode in stderr, so I’ve also made webkitpy tolerant of utf-8 in stderr, where before it only (incorrectly) expected ascii. I don’t see any more symptoms on my end. So, perhaps you wanted me to change this log message?
> 

Yes, I suggest that you update this sentence given that you feel that the proposed patch has "all of the bases covered".

[David Farler]

Created attachment 237866 [details]
Patch

[WebKit Commit Bot]

Attachment 237866 [details] did not pass style-queue:


ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:95:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 237866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237866&action=review

> Tools/ChangeLog:10
> +        unicode (encoded as utf-8) when writing to files, in the

Nit: unicode => Unicode

And

utf-8 => UTF-8

> Tools/ChangeLog:12
> +        rare case that an NSError description will make it to
> +        stderr.

For your consideration, I suggest that you add an example to this description of when an NSError with Unicode characters can be emitted to the standard error stream.

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:97
> +        [self didCrashWithMessage:nil];

I take it that there is little value to pass a string to -didCrashWithMessage that describes the reason for the NSException?

> Tools/Scripts/webkitpy/common/system/filesystem.py:228
> +            # FIXME: Just f.write(contents) when switching to Python 3

This comment isn't very useful because it doesn't explain "why" we can simplify this code. I suspect that there are many other areas of our Python code that can be simplified or will need to be updated to work with Python 3. If you choose to keep this comment I suggest elaborating why we can simplify this code in Python 3.

[David Farler]

(In reply to comment #17)
> (From update of attachment 237866 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237866&action=review
> 
> > Tools/ChangeLog:10
> > +        unicode (encoded as utf-8) when writing to files, in the
> 
> Nit: unicode => Unicode
> 
> And
> 
> utf-8 => UTF-8

Thanks - will fix.

> 
> > Tools/ChangeLog:12
> > +        rare case that an NSError description will make it to
> > +        stderr.
> 
> For your consideration, I suggest that you add an example to this description of when an NSError with Unicode characters can be emitted to the standard error stream.

Sure thing.

> 
> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:97
> > +        [self didCrashWithMessage:nil];
> 
> I take it that there is little value to pass a string to -didCrashWithMessage that describes the reason for the NSException?

In this case, no extra information is necessary. This would happen if the underlying DRT/WKTR crashed and the relevant information will get pulled from that crash log. I just added this as API for special cases but it can probably just be a void message in the future.

> 
> > Tools/Scripts/webkitpy/common/system/filesystem.py:228
> > +            # FIXME: Just f.write(contents) when switching to Python 3
> 
> This comment isn't very useful because it doesn't explain "why" we can simplify this code. I suspect that there are many other areas of our Python code that can be simplified or will need to be updated to work with Python 3. If you choose to keep this comment I suggest elaborating why we can simplify this code in Python 3.

True. In fact, this whole module will be removed, so I’ll just remove the comment and put a FIXME in my mind to remove it. :)

[David Farler]

Committed r173452: <http://trac.webkit.org/changeset/173452>