Closed Bug 961712 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: XPCOM, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: cwiiis, Assigned: cwiiis)

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Attachment #8362498 - Flags: review?(gal)
Sorry for spam, silly mistake in first patch.
Attachment #8362498 - Attachment is obsolete: true
Attachment #8362498 - Flags: review?(gal)
Attachment #8362505 - Flags: review?(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.
Attachment #8362505 - Flags: review?(gal) → review+
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
(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.
(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.
(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
Try looked good, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4e95457c87

re comment #8, this is meant to include b2g.
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
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.
This shouldn't be an issue any more because the buffering happens into stringstreams at the call sites in layout and gfx.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.