[OOPP] Plugin child aborts should trigger parent side stack generation for crash reporter

RESOLVED INCOMPLETE

Status

()

Core
Plug-ins
RESOLVED INCOMPLETE
7 years ago
5 years ago

People

(Reporter: jimm, Unassigned)

Tracking

Trunk
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
Bug 677711 introduces child side abort functionality when the child detects a hung parent. Currently we get a child side stack submitted but not the parent's. Getting this submitted will likely involve the io thread since the ui thread in the parent is not responding.
(Reporter)

Updated

7 years ago
Assignee: nobody → jmathies
(Reporter)

Comment 1

7 years ago
Created attachment 559567 [details] [diff] [review]
patch rev 1

Posting the current rev of this work as a single patch.

This is working pretty well and socorro seems happy with the extra data as it's matching the two reports up.

https://crash-stats.mozilla.com/report/index/bp-5a9b4b6f-98ad-4740-9474-e2d752110909
https://crash-stats.mozilla.com/report/index/bp-5a9b4b6f-98ad-4740-9474-e2d752110909

Unfortunately I did have to add a bit of code and make a few changes to breakpad. I'm currently looking for ways to pull this out to avoid it, but I thought I'd post what I have for now. Either way I plan on splitting up what I end up with into smaller patches and kicking off some reviews next week. I'd really like to get this landed in some form or another pretty soon.
(Reporter)

Comment 2

7 years ago
Created attachment 559568 [details] [diff] [review]
patch rev 1

- comment cleanup
Attachment #559567 - Attachment is obsolete: true
(Reporter)

Comment 3

7 years ago
Created attachment 560021 [details] [diff] [review]
breakpad v.1
Attachment #559568 - Attachment is obsolete: true
(Reporter)

Comment 4

7 years ago
Created attachment 560022 [details] [diff] [review]
ex handler v.1
(Reporter)

Comment 5

7 years ago
Created attachment 560023 [details] [diff] [review]
plugins v.1
(Reporter)

Comment 6

7 years ago
Comment on attachment 560021 [details] [diff] [review]
breakpad v.1

Breakpad changes: this adds a new event the child can trigger to generate a minidump on the parent side of the connection.
Attachment #560021 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 7

7 years ago
Comment on attachment 560022 [details] [diff] [review]
ex handler v.1

exception handler callback and xre changes to track and take parent side hangs the child requests.
Attachment #560022 - Flags: review?(benjamin)
(Reporter)

Comment 8

7 years ago
Comment on attachment 560023 [details] [diff] [review]
plugins v.1

plugin changes that tag the two minidumps into a pair for crash reporting.
Attachment #560023 - Flags: review?(benjamin)

Comment 9

7 years ago
Comment on attachment 560022 [details] [diff] [review]
ex handler v.1

Can I wait to review these until Ted approves the breakpad change?
Attachment #560022 - Flags: review?(benjamin)

Updated

7 years ago
Attachment #560023 - Flags: review?(benjamin)
Comment on attachment 560021 [details] [diff] [review]
breakpad v.1

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

Sorry, I kept looking at this patch and not having time to get through all of it. This looks good. The only thing I'd like to see is a unit test for the added functionality. Unfortunately our Breakpad test coverage for CrashGenerationServer is pretty poor, so I guess we can let that slide for now. I'll land this upstream for you.
Attachment #560021 - Flags: review?(ted.mielczarek) → review+
(Reporter)

Comment 11

7 years ago
(In reply to Ted Mielczarek [:ted, :luser] from comment #10)
> Comment on attachment 560021 [details] [diff] [review] [diff] [details] [review]
> breakpad v.1
> 
> Review of attachment 560021 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I kept looking at this patch and not having time to get through all
> of it. This looks good. The only thing I'd like to see is a unit test for
> the added functionality. Unfortunately our Breakpad test coverage for
> CrashGenerationServer is pretty poor, so I guess we can let that slide for
> now. I'll land this upstream for you.

Thanks. Note, this changes the signature of OnChildProcessDumpRequested in the exception handler code, so the ex handler v.1 patch needs to land with it on mc. Does "landing it upstream" include an mc landing, or are you referring to google's code repo, or something else?
(Reporter)

Updated

7 years ago
Attachment #560022 - Flags: review?(benjamin)
(Reporter)

Comment 12

7 years ago
Comment on attachment 560023 [details] [diff] [review]
plugins v.1

spreading these around. bent, would you mind picking up the review on this one?
Attachment #560023 - Flags: review?(bent.mozilla)
Landed upstream:
http://code.google.com/p/google-breakpad/source/detail?r=883

I had to fix a few places because we still have a local patch that hasn't been upstreamed, and also I had to fix a few of the Breakpad unittests to match the new method signatures.
By "upstream" I just meant "Breakpad SVN".
Comment on attachment 560023 [details] [diff] [review]
plugins v.1

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +300,5 @@
>  {
>      switch (why) {
>      case AbnormalShutdown: {
>  #ifdef MOZ_CRASHREPORTER
> +        nsCOMPtr<nsILocalFile> browserDump;

Nit: Since this isn't used outside the |if (TakeMinidump(getter_AddRefs(pluginDump))| block I'd move it inside.

@@ +305,2 @@
>          nsCOMPtr<nsILocalFile> pluginDump;
> +        if (TakeMinidump(getter_AddRefs(pluginDump)) && // in pending

Nit: The "in pending" comment here and below is not very descriptive...

@@ +309,5 @@
> +            // requested before purposefully aborting due to a parent side
> +            // hang.
> +            if (XRE_TakeMinidumpForParent(getter_AddRefs(browserDump)) && // in pending
> +                CrashReporter::GetIDFromMinidump(browserDump, mBrowserDumpID) &&
> +                CrashReporter::CreatePairedHangId(&mHangID)) {

The fallback is fine, I guess, but as written we won't be notified of failures in any of these methods, do you think they should be logged?
Attachment #560023 - Flags: review?(bent.mozilla) → review+

Comment 16

7 years ago
Comment on attachment 560022 [details] [diff] [review]
ex handler v.1

>diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp

>+#if defined(XP_WIN)
>+class CrashDumpSetupWin : public nsIRunnable {

nit, please put opening brace of classes on the next line
Attachment #560022 - Flags: review?(benjamin) → review+
This got backed out upstream for suspected bustage (presumably in Chrome):
http://code.google.com/p/google-breakpad/source/detail?r=891
(Reporter)

Comment 18

6 years ago
Created attachment 581793 [details] [diff] [review]
plugins v.2

Updated to all the code changes that were made to plugin crash reporting. Also working on a new breakpad patch that won't change the callback signatures, which should address any problems google had.
Attachment #560023 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Assignee: jmathies → nobody

Comment 19

5 years ago
I don't think this is going to be especially valuable.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.