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

RESOLVED WONTFIX

Status

()

Core
XPCOM
RESOLVED WONTFIX
4 years ago
3 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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)

Updated

4 years ago
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
Created attachment 8362498 [details] [diff] [review]
Buffer (f)printf_stderr between newlines on Android
Attachment #8362498 - Flags: review?(gal)
(Assignee)

Comment 2

4 years ago
Created attachment 8362505 [details] [diff] [review]
Buffer (f)printf_stderr between newlines on Android

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 3

4 years ago
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
(Assignee)

Comment 5

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/171c124a1402
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.
(Assignee)

Comment 9

4 years ago
(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
(Assignee)

Comment 10

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

re comment #8, this is meant to include b2g.
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
(Assignee)

Comment 12

4 years ago
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
(Assignee)

Comment 13

4 years ago
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
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
(See bug 1027496)
You need to log in before you can comment on or make changes to this bug.