Closed
Bug 1258663
Opened 8 years ago
Closed 8 years ago
Annotate system call failure regarding IPC transports
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: cyu, Assigned: cyu)
References
Details
Attachments
(1 file)
6.38 KB,
patch
|
gkrizsanits
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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).
Updated•8 years ago
|
Assignee: nobody → cyu
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8734655 -
Flags: review?(gkrizsanits)
Comment 2•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f71f7068c9f0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 6•8 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•8 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?
status-firefox47:
--- → affected
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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/5914dcad53c6
Assignee | ||
Comment 10•8 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•8 years ago
|
Attachment #8734655 -
Flags: approval-mozilla-beta?
You need to log in
before you can comment on or make changes to this bug.
Description
•