Add an API to annotate pending IPC when push IPC message to pending queue OOM

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ting, Assigned: ting)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
In bug 1287044 I used CrashReporter::AnnotateCrashReport() to add annotations to crash report when OOM, however it doesn't work when OOM is not happend on the main thread, because CrashReporterHelperRunnable won't have a chance to run.

So I need to add a new API (similar to AnnotateOOMAllocationSize())in nsExceptionHandler to make sure the annotations will be added to the crash report.
(Assignee)

Updated

2 years ago
Blocks: 1266517
Comment hidden (mozreview-request)
Component: General → IPC

Comment 2

2 years ago
mozreview-review
Comment on attachment 8788824 [details]
Bug 1301022 - Add an API for annotating pending IPC messages.

https://reviewboard.mozilla.org/r/77174/#review77330

::: toolkit/crashreporter/nsExceptionHandler.cpp:577
(Diff revision 1)
>    gTexturesSize = size;
>  }
>  
> +static size_t gNumOfPendingIPC = 0;
> +static uint32_t gTopPendingIPCCount = 0;
> +static const char* gTopPendingIPCName = nullptr;

This doesn't inspire me with confidence in C++, but I guess this is always a compile-time constant string in practice? (Looking at the callers of `IPC::Message::set_name`: https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22IPC%3A%3AMessage%3A%3Aset_name%28const+char+%2Aconst%29%22 )

Sure makes me wish this was Rust code and you could use `&'static str`!
Attachment #8788824 - Flags: review?(ted) → review+
Comment hidden (obsolete)
(Assignee)

Comment 4

2 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> > +static const char* gTopPendingIPCName = nullptr;
> 
> This doesn't inspire me with confidence in C++, but I guess this is always a
> compile-time constant string in practice? (Looking at the callers of

Please ignore my incorrect comment 3. So yes, and in case if it hasn't been initialized, it will be "???", see

https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/ipc/chromium/src/chrome/common/ipc_message.h#319, and
https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/ipc/chromium/src/chrome/common/ipc_message.h#59-61
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
Try: https://hg.mozilla.org/try/rev/883813acc51e9067122765c1a24f4adb750904f9

There's a change that I replaced msgid_t with uint32_t to get rid of the dependency on ipc_message.h.

Comment 7

2 years ago
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3719ffa5eb4b
Add an API for annotating pending IPC messages. r=ted

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3719ffa5eb4b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 9

2 years ago
Comment on attachment 8788824 [details]
Bug 1301022 - Add an API for annotating pending IPC messages.

Approval Request Comment
[Feature/regressing bug #]: bug 1287044
[User impact if declined]: none
[Describe test coverage new/current, TreeHerder]: current tests
[Risks and why]: low, because it just fixes crash annotations when OOM in pushing IPC message to the queue (bug 1266517)
[String/UUID change made/needed]: none
Attachment #8788824 - Flags: approval-mozilla-aurora?
Comment on attachment 8788824 [details]
Bug 1301022 - Add an API for annotating pending IPC messages.

This patch fixes crash annotations when OOM in pushing IPC message to the queue. Take it in 51 aurora.
Attachment #8788824 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: nobody → janus926

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/67474923ff42
status-firefox51: affected → fixed
(Assignee)

Comment 12

2 years ago
Even with the patch, bp-9b712aed-dd03-4b99-b181-a82a32160923 still doesn't have the annotations.

It seems the annotations also need to be added in PrepareChildExceptionTimeAnnotations() to make it work when OOM occurs in the child process.
Flags: needinfo?(ted)
(Assignee)

Comment 13

2 years ago
Pressed enter accidentally, I haven't finished comment 12.

I tested locally with PrepareChildExceptionTimeAnnotations() fixed, and made sure the annotations were present on crash-stats. But I still want to double confirm with you if I am doing correct.
(Assignee)

Comment 14

2 years ago
If my understanding is correct, I'll backout the patch from aurora/central, fix it, and land again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

2 years ago
Created attachment 8797017 [details] [diff] [review]
backout

Backout from Aurora at first.

Approval Request Comment
[Feature/regressing bug #]: no regression
[User impact if declined]: none, but the crash annotations won't work if it's crashed in child process
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: none
[String/UUID change made/needed]: none
Attachment #8797017 - Flags: approval-mozilla-aurora?
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
Comment on attachment 8788824 [details]
Bug 1301022 - Add an API for annotating pending IPC messages.

Added annotations to PrepareChildExceptionTimeAnnotations(), asked for review again to make sure I don't miss anything else.
Flags: needinfo?(ted)
Attachment #8788824 - Flags: review+ → review?(ted)

Updated

2 years ago
status-firefox51: fixed → affected
Comment on attachment 8797017 [details] [diff] [review]
backout

Back out patch as it doesn't work when OOM in child process. Take it in 51 aurora.
Attachment #8797017 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/162e2bf8a48d
status-firefox51: affected → fixed

Comment 20

2 years ago
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/841027862f31
Back out crash annotations which doesn't work in child process. r=me

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/841027862f31
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 22

2 years ago
Forgot to set leave-open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 23

2 years ago
mozreview-review
Comment on attachment 8788824 [details]
Bug 1301022 - Add an API for annotating pending IPC messages.

https://reviewboard.mozilla.org/r/77174/#review85242

Looks good, thanks!
Attachment #8788824 - Flags: review?(ted) → review+

Comment 25

2 years ago
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/373457d82832
Add an API for annotating pending IPC messages. r=ted

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/373457d82832
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Depends on: 1312326
(Assignee)

Updated

2 years ago
No longer depends on: 1312326
(Assignee)

Comment 27

2 years ago
Gerry, bp-b4853361-f3cf-4609-b155-d20b82161021 shows that the fix does work now, so I'd like to ask for uplifting again. But the flag approval‑mozilla‑aurora can't be reset, how should I proceed?
Flags: needinfo?(gchang)

Updated

2 years ago
Flags: needinfo?(gchang)
Attachment #8788824 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 28

2 years ago
Comment on attachment 8788824 [details]
Bug 1301022 - Add an API for annotating pending IPC messages.

Approval Request Comment
[Feature/regressing bug #]: bug 1287044
[User impact if declined]: none
[Describe test coverage new/current, TreeHerder]: current tests
[Risks and why]: low, because it just fixes crash annotations when OOM in pushing IPC message to the queue (bug 1266517)
[String/UUID change made/needed]: none
Attachment #8788824 - Flags: approval-mozilla-aurora?
Comment on attachment 8788824 [details]
Bug 1301022 - Add an API for annotating pending IPC messages.

Help dev debug in crash reports. Take it in 51 aurora.
Attachment #8788824 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.