Log errors during daemon IPC

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
mozilla43
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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.
Attachment #8657778 - Flags: review?(shuang)
Attachment #8657778 - Flags: feedback?(gsvelto)
Changes since v1:

  - clarified comments about when errors are logged
Attachment #8657778 - Attachment is obsolete: true
Attachment #8657778 - Flags: review?(shuang)
Attachment #8657778 - Flags: feedback?(gsvelto)
Attachment #8657787 - Flags: review?(shuang)
Attachment #8657787 - Flags: feedback?(gsvelto)
Changes since v1:

  - fixed logging in |PackPDU(BluetoothNamedValue)|
Attachment #8657781 - Attachment is obsolete: true
Attachment #8657781 - Flags: review?(shuang)
Attachment #8657788 - Flags: review?(shuang)
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.
Attachment #8657787 - Flags: review?(shuang)
(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.
Changes since v2:

  - using MOZ_CRASH to abort Gecko process
Attachment #8657787 - Attachment is obsolete: true
Attachment #8657787 - Flags: feedback?(gsvelto)
Attachment #8658218 - Flags: review?(shuang)
Attachment #8658218 - Flags: feedback?(gsvelto)
Blocks: 1202704
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)
Attachment #8658218 - Flags: feedback?(gsvelto) → feedback+
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.
Attachment #8657780 - Flags: review?(shuang) → review+
Comment on attachment 8657788 [details] [diff] [review]
[03] Bug 1202386: Output clear Bluetooth IPC errors (v2)

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

LGTM.
Attachment #8657788 - Flags: review?(shuang) → review+
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?
Flags: needinfo?(shuang)
Keywords: leave-open
Thanks Shawn and sorry again for this faux pas.
Flags: needinfo?(shuang)
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.