Annotate system call failure regarding IPC transports

RESOLVED FIXED in Firefox 47

Status

()

Core
IPC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cyu, Assigned: cyu)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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

2 years ago
Blocks: 1258317
Assignee: nobody → cyu
(Assignee)

Comment 1

2 years ago
Created attachment 8734655 [details] [diff] [review]
Annotate IPC transport syscall failures
Attachment #8734655 - Flags: review?(gkrizsanits)
(Assignee)

Updated

2 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+
(Assignee)

Comment 3

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

Comment 4

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f71f7068c9f0de5d9a006c67c4d1e397dc3b0b0b
Bug 1258663 - Crash annotate system call failures in the IPC transport. r=gabor

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f71f7068c9f0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Comment 6

2 years ago
hey cyu, are we seeing any data yet from this on nightly? Also, can we uplift this to 47?
Flags: needinfo?(cyu)
(Assignee)

Comment 7

2 years ago
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?

Updated

2 years ago
status-firefox47: --- → affected

Comment 8

2 years ago
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 9

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/5914dcad53c6
status-firefox47: affected → fixed
(Assignee)

Comment 10

2 years ago
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

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