Closed Bug 1258663 Opened 8 years ago Closed 8 years ago

Annotate system call failure regarding IPC transports

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: cyu, Assigned: cyu)

References

Details

Attachments

(1 file)

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).
Blocks: 1258317
Assignee: nobody → cyu
Attachment #8734655 - Flags: review?(gkrizsanits)
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.
https://hg.mozilla.org/mozilla-central/rev/f71f7068c9f0
Status: NEW → RESOLVED
Closed: 8 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?
Attachment #8734655 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.