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

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Ting-Yu Chou [:ting] (away)]

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.

Comment 1

[Ting-Yu Chou [:ting] (away)]

Attachment: Bug 1301022 - Add an API for annotating pending IPC messages.

Review commit: https://reviewboard.mozilla.org/r/77174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77174/

Comment 2

[(not currently active) Ted Mielczarek]

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`!

Comment 3

[Ting-Yu Chou [:ting] (away)]

(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
> `IPC::Message::set_name`:

Here it's a string of nsPrintfCString, use a pointer so there won't be memory allocation.

Comment 4

[Ting-Yu Chou [:ting] (away)]

(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 5

[Ting-Yu Chou [:ting] (away)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77174/diff/1-2/

Comment 6

[Ting-Yu Chou [:ting] (away)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3719ffa5eb4b
Add an API for annotating pending IPC messages. r=ted

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3719ffa5eb4b

Comment 9

[Ting-Yu Chou [:ting] (away)]

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

Comment 10

[Gerry Chang [:gchang]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/67474923ff42

Comment 12

[Ting-Yu Chou [:ting] (away)]

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.

Comment 13

[Ting-Yu Chou [:ting] (away)]

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.

Comment 14

[Ting-Yu Chou [:ting] (away)]

If my understanding is correct, I'll backout the patch from aurora/central, fix it, and land again.

Comment 15

[Ting-Yu Chou [:ting] (away)]

Attachment: 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

Comment 16

[Ting-Yu Chou [:ting] (away)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77174/diff/2-3/

Comment 17

[Ting-Yu Chou [:ting] (away)]

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.

Comment 18

[Gerry Chang [:gchang]]

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.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/162e2bf8a48d

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/841027862f31
Back out crash annotations which doesn't work in child process. r=me

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/841027862f31

Comment 22

[Ting-Yu Chou [:ting] (away)]

Forgot to set leave-open.

Comment 23

[(not currently active) Ted Mielczarek]

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!

Comment 24

[Ting-Yu Chou [:ting] (away)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1175841277bc

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/373457d82832
Add an API for annotating pending IPC messages. r=ted

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/373457d82832

Comment 27

[Ting-Yu Chou [:ting] (away)]

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?

Comment 28

[Ting-Yu Chou [:ting] (away)]

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

Comment 29

[Gerry Chang [:gchang]]

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.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9bfa86a9d4d5