961712 - (f)printf_stderr on Android append new-lines to all output

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Chris Lord [:cwiiis]]

It's pretty much impossibly to read layout debugging output because Android logging functions append newlines to all output and we rely on being able to print partial lines.

Patch incoming that adds manual buffering to make output more readable.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Buffer (f)printf_stderr between newlines on Android

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Buffer (f)printf_stderr between newlines on Android

Sorry for spam, silly mistake in first patch.

Comment 3

[Andreas Gal :gal]

Comment on attachment 8362505 [details] [diff] [review]
Buffer (f)printf_stderr between newlines on Android

Can you do me a favor and test the overflow cases (too long string in fmt, too long string in %s, multiple strings overflowing the buffer) at least by hand? Thanks.

Comment 4

[Benoit Girard (:BenWa)]

Please keep in mind that ADB will silently drop lines if you post too much data in a small ammount of time (like trying to log a data base64 image encoding).

See:
https://groups.google.com/d/msg/android-developers/NDI4lusmr40/C02xITiOUAUJ

Comment 5

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #4)
> Please keep in mind that ADB will silently drop lines if you post too much
> data in a small ammount of time (like trying to log a data base64 image
> encoding).
> 
> See:
> https://groups.google.com/d/msg/android-developers/NDI4lusmr40/C02xITiOUAUJ

This is good to know... Though kinda lame.

Comment 6

[Chris Lord [:cwiiis]]

(In reply to Andreas Gal :gal from comment #3)
> Comment on attachment 8362505 [details] [diff] [review]
> Buffer (f)printf_stderr between newlines on Android
> 
> Can you do me a favor and test the overflow cases (too long string in fmt,
> too long string in %s, multiple strings overflowing the buffer) at least by
> hand? Thanks.

Tested the overflow cases manually, they worked correctly (well, the output was what was expected and there was no crashing - I haven't gone so far as to valgrind or something like that, but I think we'll be ok :)). Will push to inbound, thanks.

Comment 7

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/171c124a1402

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7c2217054821

Apparently b2g is included in #elif defined(ANDROID)? https://tbpl.mozilla.org/php/getParsedLog.php?id=33293602&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=33293485&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=33295020&tree=Mozilla-Inbound, it seems not to like newlines.

Comment 9

[Chris Lord [:cwiiis]]

(In reply to Phil Ringnalda (:philor) from comment #8)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7c2217054821
> 
> Apparently b2g is included in #elif defined(ANDROID)?
> https://tbpl.mozilla.org/php/getParsedLog.php?id=33293602&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=33293485&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=33295020&tree=Mozilla-
> Inbound, it seems not to like newlines.

ugh, off-by-one in the memmove (needs a +1, as strlen of course doesn't include the null byte). Either way, I'm pushing to try before pushing to inbound this time!

https://tbpl.mozilla.org/?tree=Try&rev=3e247a08c91d

Comment 10

[Chris Lord [:cwiiis]]

Try looked good, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4e95457c87

re comment #8, this is meant to include b2g.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This apparently broke B2G debug tests, so it was backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/13fd696b1102

sample failures: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=33420867&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=33421049&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=33420836&tree=Mozilla-Inbound

Comment 12

[Chris Lord [:cwiiis]]

I also wasn't handling the possible error return of vsnprintf, which could corrupt the buffer index... Given it's always the same caller that triggers the crash, this seems reasonably likely. Here's a try push with that fixed and hopefully trying the correct things: https://tbpl.mozilla.org/?tree=Try&rev=888fe19a0c04

Comment 13

[Chris Lord [:cwiiis]]

So, the very obvious reason this didn't work is that the code is not at all thread-safe. I'll go back and fix this when I have some spare time.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

This shouldn't be an issue any more because the buffering happens into stringstreams at the call sites in layout and gfx.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(See bug 1027496)