Annotate system call failure regarding IPC transports

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cyu, Assigned: cyu)

Tracking

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed)

Details

Attachments

(1 attachment)

in ipc/glue/Transport_win.cpp, we have several ::DuplicateHandle() calls that are asserted to succeed, and we see many crashes regarding such failures. We need to annotate the failures to see if these failures can be handled gracefully or are just out of our control (e.g. running out of resource).
Assignee

Updated

3 years ago
Blocks: 1258317
Assignee: nobody → cyu
Assignee

Updated

3 years ago
Blocks: 1253575
Comment on attachment 8734655 [details] [diff] [review]
Annotate IPC transport syscall failures

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

This is OK as it is I guess, but if you have some more time I think I would prefer to have instead of GetSystemError an AnnotateSystemError function. This way we wouldn't have to copy-paste that #ifdef code at every place we need this. It's just a plain static function so if it turns out to be a noop the compiler will opt-it out in an opt build anyway, so I would not bother inventing a macro for calling it.
Attachment #8734655 - Flags: review?(gkrizsanits) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> Comment on attachment 8734655 [details] [diff] [review]
> Annotate IPC transport syscall failures
> 
> Review of attachment 8734655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is OK as it is I guess, but if you have some more time I think I would
> prefer to have instead of GetSystemError an AnnotateSystemError function.
> This way we wouldn't have to copy-paste that #ifdef code at every place we
> need this. It's just a plain static function so if it turns out to be a noop
> the compiler will opt-it out in an opt build anyway, so I would not bother
> inventing a macro for calling it.

Thanks for the suggestion. I'll make the change with AnnotateSystemError() in the final patch. Instead of having a noop static function when MOZ_CRASHREPORTER isn't defined, I'll use the do {} while (0) to make the call a noop.

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f71f7068c9f0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
hey cyu, are we seeing any data yet from this on nightly? Also, can we uplift this to 47?
Flags: needinfo?(cyu)
Comment on attachment 8734655 [details] [diff] [review]
Annotate IPC transport syscall failures

Approval Request Comment
[Feature/regressing bug #]:
No bug. e10s internals handling IPC transports that assert on system call that could potentially fail due to unknown reason that is not controlled by us.
[User impact if declined]:
Difficulties in proceeding with some crashes that are seen by the users but hard to reproduce/diagnose/debug.
[Describe test coverage new/current, TreeHerder]:
Just landed/tested on m-c.
[Risks and why]: 
Pretty low since the patch contains only diagnostic code during crashes.
[String/UUID change made/needed]:
none
Flags: needinfo?(cyu)
Attachment #8734655 - Flags: approval-mozilla-aurora?
Comment on attachment 8734655 [details] [diff] [review]
Annotate IPC transport syscall failures

Improves our ability to diagnose e10s crashes, Aurora47+
Attachment #8734655 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8734655 [details] [diff] [review]
Annotate IPC transport syscall failures


Approval Request Comment
[Feature/regressing bug #]:
No bug. e10s internals handling IPC transports that assert on system call that could potentially fail due to unknown reason that is not controlled by us.
[User impact if declined]:
Difficulties in proceeding with some crashes that are seen by the users but hard to reproduce/diagnose/debug.
[Describe test coverage new/current, TreeHerder]:
Just landed/tested on m-c.
[Risks and why]: 
Pretty low since the patch contains only diagnostic code during crashes.
[String/UUID change made/needed]:
none
Attachment #8734655 - Flags: approval-mozilla-beta?
Assignee

Updated

3 years ago
Attachment #8734655 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.