788512 - Add dumps for Flash processes to modified plugin hang report

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Bug 784145 changes the way the dumps for plugin hangs are submitted. 
The dump submission for the Flash processes still has to be adopted to that.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Add dumps for Flash processes

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Add dumps for Flash processes to hang reports

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

Comment 3

[Ryan VanderMeulen [:RyanVM]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=dd3f0b831afe

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b14f17141bb1

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/b14f17141bb1

Comment 5

[Benjamin Smedberg]

Did you test this? I have not received any flash dumps on the new hang server... do you have crash IDs I can look at to see why it's not working?

Comment 6

[Georg Fritzsche [:gfritzsche]]

Checked into this: I didn't properly test it, as it turns out there are no dumps generated yet for the Flash child (as i assumed) and hence TakeMinidumpForChild() doesn't return anything useful. 
Investigating how to trigger dump generation for those childs.

Comment 7

[Benjamin Smedberg]

Ah indeed, "TakeMinidumpForChild" doesn't actually create a minidump, it just gets one that already exists. You basically need to create a call like which will create the minidump and name it what you want http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp?mark=2704-2718#2692

Comment 8

[Georg Fritzsche [:gfritzsche]]

Breakpad seems to require a target thread whenever a minidump is to be taken for a child. For the plugin-container we already get one via its IPC setup, but not for the Flash processes.
The best i came up with for now is the hackish approach of finding the "main" threads for them and using that (still have to clean it up though). Does the report bp-hr-20120926-3ab9dd12-34bc-489b-80dd-81ea086a0b51 look roughly sensible on the server side?

Comment 9

[Benjamin Smedberg]

No:

2012-09-26 11:43:14: minidump_processor.cc:264: INFO: Processing minidump in file /home/bsmedberg/hangprocessor/data/2012/09-26/hr-20120926-3ab9dd12-34bc-489b-80dd-81ea086a0b51/minidump_flash2.dmp
2012-09-26 11:43:14: minidump.cc:3775: INFO: Minidump opened minidump /home/bsmedberg/hangprocessor/data/2012/09-26/hr-20120926-3ab9dd12-34bc-489b-80dd-81ea086a0b51/minidump_flash2.dmp
2012-09-26 11:43:14: minidump.cc:3820: INFO: Minidump not byte-swapping minidump
2012-09-26 11:43:14: minidump.cc:4186: INFO: GetStream: type 1197932545 not present
2012-09-26 11:43:14: minidump.cc:4186: INFO: GetStream: type 1197932546 not present
2012-09-26 11:43:14: minidump.cc:1958: INFO: MinidumpModule could not determine version for C:\Program Files (x86)\NVIDIA Corporation\CoProcManager\detoured.dll
2012-09-26 11:43:14: minidump.cc:1857: INFO: MinidumpModule could not determine debug_file for C:\Program Files (x86)\WinSplit Revolution\WinSplitHook32.DLL
2012-09-26 11:43:14: minidump.cc:1926: INFO: MinidumpModule could not determine debug_identifier for C:\Program Files (x86)\WinSplit Revolution\WinSplitHook32.DLL
2012-09-26 11:43:14: minidump.cc:1322: ERROR: MinidumpThread has a memory region problem, 0x0+0x0
2012-09-26 11:43:14: minidump.cc:1516: ERROR: MinidumpThreadList cannot read thread 0/12
2012-09-26 11:43:14: minidump.cc:4209: ERROR: GetStream could not read stream type 3
2012-09-26 11:43:14: minidump_processor.cc:112: ERROR: Minidump /home/bsmedberg/hangprocessor/data/2012/09-26/hr-20120926-3ab9dd12-34bc-489b-80dd-81ea086a0b51/minidump_flash2.dmp has no thread list
2012-09-26 11:43:14: minidump.cc:3747: INFO: Minidump closing minidump
2012-09-26 11:43:14: minidump_stackwalk.cc:529: ERROR: MinidumpProcessor::Process failed

[minidump-stackwalk failed with code 1]

Comment 10

[(not currently active) Ted Mielczarek]

This sounds like the issue that bc was hitting. I had a sort-of-useful patch to work around this here:
https://breakpad.appspot.com/413002/

Comment 11

[Georg Fritzsche [:gfritzsche]]

Attachment: Generate Flash process dumps on hang

Current approach on generating minidumps: give breakpad the thread id of the oldest thread for the respective Flash process.

Comment 12

[Georg Fritzsche [:gfritzsche]]

The dumps open fine locally in Visual Studio by the way, but only show the active function in the call stack and no locals etc.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 665112 [details] [diff] [review]
Generate Flash process dumps on hang

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

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +2686,5 @@
> +  if (task_threads(childPid, &threads_for_task, &thread_count)
> +      == KERN_SUCCESS && childBlamedThread < thread_count) {
> +    childThread = threads_for_task[childBlamedThread];
> +  }
> +#else

We should probably pull this out into a separate function since it's used elsewhere.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Attachment: Output of patched dump processor

Tested around with the thread ids (check for threads being alive, set them to 0, ...) and haven't seen any difference.

Patching the hang processor prevents aborting the dump processing completely, but all threads stacks are broken. 
Attaching output of patched hang processor with the flash thread ids set to 0.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Attachment: Generate Flash process dumps on hang, v2

Turns out that the process handle used for the dump generation was missing PROCESS_VM_READ.

This might still intermittently trigger the bug Ted referenced, i'll need to check that tomorrow to be safe.

Comment 16

[Georg Fritzsche [:gfritzsche]]

While the dumps are now properly taken:
https://crash-analysis.mozilla.com/hang-reports/2012/10-11/hr-20121011-fb574736-2e41-4235-8e44-d6c9b4f36d51.html
https://crash-analysis.mozilla.com/hang-reports/2012/10-11/hr-20121011-8f2c4f02-aade-48be-a8ed-5e4ba47183a9.html

... i often get crash reports instead of hang reports:
https://crash-stats.mozilla.com/report/index/bp-fa74a1e3-9362-4183-9b94-d41952121011
https://crash-stats.mozilla.com/report/index/bp-e1832d18-f65e-45e2-96cb-93b502121011

Looks like the .extra annotations for the hang are not added for some reason.

Comment 17

[Georg Fritzsche [:gfritzsche]]

The write-out of the .extra annotations is triggered via:
-> AsyncChannel::Close()
-> AsyncChannel::NotifyMaybeChannelError()
[...]
-> PluginModuleParent::ActorDestroy(AbnormalShutdown)
-> PluginModuleParent::ProcessFirstMinidump()
-> CrashReporterParent::GenerateChildData()

When the plugin .extra data is missing, those are never called.

Comment 18

[Benjamin Smedberg]

When those aren't called, we shouldn't be getting a plugin hang notification at all in the UI. Are you submitting these via about:crashes?

The codepath is supposed to be:

* at the end of PluginModuleParent::ShouldContinueFromReplyTimeout(): post PluginModuleParent::CleanupFromTimeout
* in CleanupFromTimeout, Close(), but only if OkToCleanup! This may be the problem, in which case we should repost CleanupFromTimeout the same way we repost NotifyPluginCrashed

Comment 19

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> When those aren't called, we shouldn't be getting a plugin hang notification
> at all in the UI. Are you submitting these via about:crashes?

Yes and yes.

> * at the end of PluginModuleParent::ShouldContinueFromReplyTimeout(): post
> PluginModuleParent::CleanupFromTimeout
> * in CleanupFromTimeout, Close(), but only if OkToCleanup! This may be the
> problem, in which case we should repost CleanupFromTimeout the same way we
> repost NotifyPluginCrashed

Yes, that fixes it. So this is a separate issue and actually goes back to bug 747055.

Comment 20

[Georg Fritzsche [:gfritzsche]]

Attachment: Repost cleanup notification

Repost PluginModuleParent::CleanupFromTimeout() to avoid skipping it.

Comment 21

[Georg Fritzsche [:gfritzsche]]

Attachment: Generate Flash process dumps on hang, v3

Comment 22

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 670884 [details] [diff] [review]
Repost cleanup notification

Note bug 747055, comments 34-35.

Comment 23

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> Comment on attachment 670884 [details] [diff] [review]
> Repost cleanup notification

Also worth noting that plugin hang notifications and reports are covered by tests, but they never triggered on such a scenario as far as i know.

Comment 24

[Benjamin Smedberg]

Comment on attachment 670887 [details] [diff] [review]
Generate Flash process dumps on hang, v3

lgtm, let's see if ted wants to review the crashreporter bits of this

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 670887 [details] [diff] [review]
Generate Flash process dumps on hang, v3

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

This looks fine, just one nit.

::: toolkit/crashreporter/nsExceptionHandler.h
@@ +122,5 @@
> +// Create an additional minidump for a child of a process which already has
> +// a minidump (|parentMinidump|).
> +// The resulting dump will get the id of the parent and use the |name| as
> +// an extension.
> +bool CreateChildMinidump(ProcessHandle childPid,

Looking at this again, it might be better to call this "CreateAdditionalChildMinidump", just to be very explicit. It's not really a deal-breaker though.

Comment 26

[Georg Fritzsche [:gfritzsche]]

Attachment: Generate Flash process dumps on hang, v4

Adressed review comments.

Comment 27

[Georg Fritzsche [:gfritzsche]]

Attachment: Repost cleanup notification

Comment 28

[Georg Fritzsche [:gfritzsche]]

Attachment: Generate Flash process dumps on hang, v5

Fix Linux build bustage for newer breakpad revision.
Try: https://tbpl.mozilla.org/?tree=Try&rev=0a6ae5301535

Comment 29

[Benjamin Smedberg]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8926a55f661
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b96e7442087

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b96e7442087
https://hg.mozilla.org/mozilla-central/rev/c8926a55f661

Comment 31

[Benjamin Smedberg]

Comment on attachment 671370 [details] [diff] [review]
Repost cleanup notification

[Approval Request Comment]
This change is a feature uplift which allows plugin hang reports to be more useful. The code in question only runs when we've already detected that a plugin has stopped responding, and this appears relatively safe. The only risk I can see is that we'd end up causing some kind of crash in the Firefox process while running this code, but that would show up on crash-stats pretty quickly/obviously.

Comment 32

[Benjamin Smedberg]

https://hg.mozilla.org/releases/mozilla-aurora/rev/90c5f5573cc0
https://hg.mozilla.org/releases/mozilla-aurora/rev/709a4e99bb08