852117 - Tag nested eventloops so they show up in crashstats

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[(dormant account)]

It would be good to get an idea whether nested event loops increase probability of certain crashes

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: WIP

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Adds CrashReporter annotations for nested event loops

This patch moves nesting level increment/decrement into a function so that we can update the crash reporter with the new nesting level when it changes.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Try build:
https://tbpl.mozilla.org/?tree=Try&rev=624647310b6c

Push to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/794d86e866c3

Comment 4

[Justin Dolske [:Dolske]]

Hmm, shouldn't this be attempting to only annotate when the event loop depth is >= 2? I'd worry about being surprised (now or in the future) by having crashreporter code suddenly causing a performance regression by making the main (hot) event loop slow.

Comment 5

[Josh Matthews [:jdm]]

The annotation for >1 won't go away unless you reannotate with 1 once the nesting disappears.

Comment 6

[Justin Dolske [:Dolske]]

Sure, of course when the nesting goes from 2->1 the annotation should be updated. I'm saying we are calling these functions over and over when it's going from 0->1 and 1->0, and that's pure overhead that isn't useful.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

I do agree about performance regression concerns; I'm already receiving Talos mail regarding the patch on inbound; it increased the malloc count by 3.5% to 4.5% depending on platform, causing a very noticeable blip on the graph, for example:
http://mzl.la/YdgIfE

I've noticed that the implementation of AnnotateCrashReport involves an enumeration of the annotation hash table and numerous string concatenations on every call; probably not something we want in the event loop hot path. ;-)

Backed out for now due to perf regressions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6471aba7776

Comment 8

[Ryan VanderMeulen [:RyanVM]]

FYI, pretty sure this was causing infrequent mochitest-2 leaks as well. At least, I could get them to happen on the push prior to yours.
https://tbpl.mozilla.org/php/getParsedLog.php?id=21127036&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=21127706&tree=Mozilla-Inbound

Comment 9

[Ryan VanderMeulen [:RyanVM]]

s/could/couldn't, of course

Comment 10

[Phil Ringnalda (:philor)]

Keep a close eye on Windows xpcshell and cross-platform mochitest-2 crashes in your Try pushes, too - there was a rash of both while you were in the tree, which seems to have gone away with your backout.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

I suspect that the mochitest-2 leaks were instances of bug 856600 but this patch amplified that bug's effects.

Comment 12

[(not currently active) Ted Mielczarek]

My bug 860505 comment 1 stands here. We typically just add special-cases for things that need to be annotated in hot or unsafe code paths.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add event loop nesting level API to crash reporter

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Modify nsBaseAppShell to notify the crash reporter when the event loop nesting level changes (rev.2)

This patch moves nesting level increment/decrement into a function so that we can update the crash reporter with the new nesting level when it changes.

Unlike the previous version of this patch, we now call the new CrashReporter::SetEventloopNestingLevel function as implemented in Part 1 instead of calling CrashReporter::AnnotateCrashReport.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 8373774 [details] [diff] [review]
Part 1: Add event loop nesting level API to crash reporter

Review of attachment 8373774 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +590,5 @@
>  
> +      if (eventloopNestingLevel > 0) {
> +        WriteFile(hFile, kEventLoopNestingLevelParameter, kEventLoopNestingLevelParameterLen,
> +                  &nBytes, nullptr);
> +        int numChars = _snprintf(buffer, sizeof(buffer), "%u\n", eventloopNestingLevel);

You probably don't want to use snprintf on Windows, it doesn't actually work right. You can use ltoa like XP_TTOA does for 32-bit time_t.

@@ +699,5 @@
>        }
> +      if (eventloopNestingLevel > 0) {
> +        unused << sys_write(fd, kEventLoopNestingLevelParameter, kEventLoopNestingLevelParameterLen);
> +        char buffer[16];
> +        int numChars = snprintf(buffer, sizeof(buffer), "%u\n", eventloopNestingLevel);

You're not allowed to call stdlib functions on Linux, it's potentially crashy in an exception handler. You can cheat and just use XP_TTOA here.

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add event loop nesting level API to crash reporter (v2)

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15)
> Comment on attachment 8373774 [details] [diff] [review]
> Part 1: Add event loop nesting level API to crash reporter
> 
> Review of attachment 8373774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/crashreporter/nsExceptionHandler.cpp
> @@ +590,5 @@
> >  
> > +      if (eventloopNestingLevel > 0) {
> > +        WriteFile(hFile, kEventLoopNestingLevelParameter, kEventLoopNestingLevelParameterLen,
> > +                  &nBytes, nullptr);
> > +        int numChars = _snprintf(buffer, sizeof(buffer), "%u\n", eventloopNestingLevel);
> 
> You probably don't want to use snprintf on Windows, it doesn't actually work
> right. You can use ltoa like XP_TTOA does for 32-bit time_t.

Fixed using _ultoa.

> 
> @@ +699,5 @@
> >        }
> > +      if (eventloopNestingLevel > 0) {
> > +        unused << sys_write(fd, kEventLoopNestingLevelParameter, kEventLoopNestingLevelParameterLen);
> > +        char buffer[16];
> > +        int numChars = snprintf(buffer, sizeof(buffer), "%u\n", eventloopNestingLevel);
> 
> You're not allowed to call stdlib functions on Linux, it's potentially
> crashy in an exception handler. You can cheat and just use XP_TTOA here.

Fixed as suggested.

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 8375173 [details] [diff] [review]
Part 1: Add event loop nesting level API to crash reporter (v2)

Review of attachment 8375173 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #17)
> Comment on attachment 8375173 [details] [diff] [review]
> Part 1: Add event loop nesting level API to crash reporter (v2)
> 
> Review of attachment 8375173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks.

Did you mean to mark that as r+?

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 8375173 [details] [diff] [review]
Part 1: Add event loop nesting level API to crash reporter (v2)

Review of attachment 8375173 [details] [diff] [review]:
-----------------------------------------------------------------

Uh, yes. Oops!

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Try: https://tbpl.mozilla.org/?tree=Try&rev=8f15ef0c013e

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9faf52a7d5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/af983860c74d

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9faf52a7d5f
https://hg.mozilla.org/mozilla-central/rev/af983860c74d