Closed Bug 1301022 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: ting, Assigned: ting)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Blocks: 1266517
Component: General → IPC
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+
(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
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.
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3719ffa5eb4b
Add an API for annotating pending IPC messages. r=ted
https://hg.mozilla.org/mozilla-central/rev/3719ffa5eb4b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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
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)
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.
If my understanding is correct, I'll backout the patch from aurora/central, fix it, and land again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch backoutSplinter Review
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 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)
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+
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
https://hg.mozilla.org/mozilla-central/rev/841027862f31
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Forgot to set leave-open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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+
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/373457d82832
Add an API for annotating pending IPC messages. r=ted
https://hg.mozilla.org/mozilla-central/rev/373457d82832
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Depends on: 1312326
No longer depends on: 1312326
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)
Flags: needinfo?(gchang)
Attachment #8788824 - Flags: approval-mozilla-aurora+
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.