Closed Bug 679238 Opened 9 years ago Closed 7 years ago

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

Categories

(Core :: Plug-ins, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: jimm, Unassigned)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Assignee: nobody → jmathies
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Attached patch patch rev 1 (obsolete) — Splinter Review
- comment cleanup
Attachment #559567 - Attachment is obsolete: true
Attached patch breakpad v.1Splinter Review
Attachment #559568 - Attachment is obsolete: true
Attached patch ex handler v.1Splinter Review
Attached patch plugins v.1 (obsolete) — Splinter Review
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)
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)
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 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)
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+
(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?
Attachment #560022 - Flags: review?(benjamin)
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 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
Attached patch plugins v.2Splinter Review
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
Assignee: jmathies → nobody
I don't think this is going to be especially valuable.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.