1202386 - Log errors during daemon IPC

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The review in bug 1194721 brought up the issue of logging errors in the IPC with daemon processes. This bug report is about landing patches to add logging to the respective source files.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1202386: Add logging macros for HAL IPC

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1202386: Output clear HAL IPC errors

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1202386: Output clear Bluetooth IPC errors

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1202386: Add logging macros for HAL IPC (v2)

Changes since v1:

  - clarified comments about when errors are logged

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1202386: Output clear Bluetooth IPC errors (v2)

Changes since v1:

  - fixed logging in |PackPDU(BluetoothNamedValue)|

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8657787 [details] [diff] [review]
[01] Bug 1202386: Add logging macros for HAL IPC (v2)

Review of attachment 8657787 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/hal/DaemonSocketPDUHelpers.cpp
@@ +20,3 @@
>  #include <android/log.h>
> +
> +#define CHROMIUM_LOG(args...) \

After checking bug 951207, calling it CHROMIUM_LOG is interesting. It's different from CHROMIUM_LOG "chromium/src/base/logging.h". But I guess it's not important anyway. :)

@@ +54,5 @@
> +  CHROMIUM_LOG_VA(aFmt, ap);
> +  va_end(ap);
> +
> +  if (MOZ_HAL_ABORT_ON_IPC_ERRORS) {
> +    NS_ABORT();

https://ask.mozilla.org/question/723/how-to-choose-among-ns_runtimeabort-moz_crash-ns_assertion-and-moz_assert/

I prefer to use MOZ_CRASH.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> > +#define CHROMIUM_LOG(args...) \
> 
> After checking bug 951207, calling it CHROMIUM_LOG is interesting. It's
> different from CHROMIUM_LOG "chromium/src/base/logging.h". But I guess it's
> not important anyway. :)

That was introduced by qDot back in the early BlueZ days. And it has been copied and re-used since then. ;)

> @@ +54,5 @@
> > +  CHROMIUM_LOG_VA(aFmt, ap);
> > +  va_end(ap);
> > +
> > +  if (MOZ_HAL_ABORT_ON_IPC_ERRORS) {
> > +    NS_ABORT();
> 
> https://ask.mozilla.org/question/723/how-to-choose-among-ns_runtimeabort-
> moz_crash-ns_assertion-and-moz_assert/
> 
> I prefer to use MOZ_CRASH.

I didn't know that, thanks for the pointer.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1202386: Add logging macros for HAL IPC (v3)

Changes since v2:

  - using MOZ_CRASH to abort Gecko process

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8658218 [details] [diff] [review]
[01] Bug 1202386: Add logging macros for HAL IPC (v3)

Review of attachment 8658218 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, nice that this stuff could be factored easily (and yeah, the CHROMIUM_LOG bit was a bit weird)

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8657780 [details] [diff] [review]
[02] Bug 1202386: Output clear HAL IPC errors

Review of attachment 8657780 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Sorry for late reply.

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8657788 [details] [diff] [review]
[03] Bug 1202386: Output clear Bluetooth IPC errors (v2)

Review of attachment 8657788 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/c0e2d913c601
https://hg.mozilla.org/integration/b2g-inbound/rev/f353659e96a7
https://hg.mozilla.org/integration/b2g-inbound/rev/ac0a29169cc9

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Oh, damn it. Landed this before [01] had an r+. :( I somehow thought that was the case. Sorry!

Shawn, can you check [01] and I'll fix any complaints in an additional patch?

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Try is here 

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=ac0a29169cc9

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks Shawn and sorry again for this faux pas.

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c0e2d913c601
https://hg.mozilla.org/mozilla-central/rev/f353659e96a7
https://hg.mozilla.org/mozilla-central/rev/ac0a29169cc9