Closed Bug 852117 Opened 7 years ago Closed 6 years ago

Tag nested eventloops so they show up in crashstats

Categories

(Firefox :: General, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: taras.mozilla, Assigned: aklotz)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 3 obsolete files)

It would be good to get an idea whether nested event loops increase probability of certain crashes
Assignee: nobody → aklotz
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #728279 - Attachment is obsolete: true
Attachment #729135 - Flags: review?(roc)
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.
The annotation for >1 won't go away unless you reannotate with 1 once the nesting disappears.
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.
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
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
s/could/couldn't, of course
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.
I suspect that the mochitest-2 leaks were instances of bug 856600 but this patch amplified that bug's effects.
Depends on: 856600
Depends on: 860505
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.
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.
Attachment #729135 - Attachment is obsolete: true
Attachment #8373775 - Flags: review?(roc)
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.
Attachment #8373774 - Flags: review?(ted) → review-
(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.
Attachment #8373774 - Attachment is obsolete: true
Attachment #8375173 - Flags: review?(ted)
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.
(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+?
Flags: needinfo?(ted)
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!
Attachment #8375173 - Flags: review?(ted) → review+
Flags: needinfo?(ted)
https://hg.mozilla.org/mozilla-central/rev/d9faf52a7d5f
https://hg.mozilla.org/mozilla-central/rev/af983860c74d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.