784145 - Change hang report submission to submit all minidumps at once

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

See https://wiki.mozilla.org/Socorro/Hang_Processing_Proposal for context

When submitting a hang report, don't make two different submissions: instead make one submission with multiple minidumps.

Please submit the plugin-container minidump using the existing field name (upload_file_minidump).

Submit a new text field "additional_minidumps=name1,name2"

And then submit the additional minidumps with field names "upload_file_minidump_<name1>".

At first we should submit only one additional minidump the browser stack. I'll file a separate bug to also submit dumps for the two additional Flash processes (broker and sandbox).

Comment 1

[Benjamin Smedberg]

Note: doing this as specced should continue to correctly submit into socorro, *however*: the extra minidumps will get stored into the raw .json where they are not wanted. I'm working on a minimal socorro patch to filter out the extras which I hope to push into socorro release on a quick schedule so that we can safely push this through the Firefox trains.

gfritzsche, want to take this?

Comment 2

[(not currently active) Ted Mielczarek]

This would involve some changes to CrashSubmit.jsm to allow submitting multiple dumps:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/CrashSubmit.jsm

as well as the chrome code that handles plugin hangs, to submit them all in one go.

Comment 3

[[github robot]]

Commit pushed to master at https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/773137de91466fb20f85d957c67f0407614738e6
Filter out unexpected upload files from the list of items saved in the raw .json, in preparation for client-side changes in bug 784145.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Modify hang submission

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

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 656946 [details] [diff] [review]
Modify hang submission

Green on try except for unrelated oranges.

Comment 6

[Benjamin Smedberg]

I don't think this is actually how we want to accomplish this. It accomplishes the current goal (submitting two minidumps) in a fairly simple manner, but it's going to be a lot more complicated to submit the two additional Flash minidumps, because we'd have to attach them to the plugin-crashed event.

Instead, I think that the browser-plugins code should be aware of only one crash ID. The code in CrashReporter::CreatePairedMinidumps should refactored to take the plugin minidump. Maybe another method should take the "browser" minidump and move/rename it to <childid>_browser.dmp. Then the Flash dumps will be <childid>_flash1.dmp and <childid>_flash2.dmp. And we'll just add the .extra annotation from http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginModuleParent.cpp#296

Comment 7

[Benjamin Smedberg]

Attachment: Crash reporter/plugins support for including multiple minidumps in one crash report

Here's part of the solution; this should provide the backend crashreporter/plugin support for submitting the browser minidump as part of a single crash report. It will still need CrashSubmit.jsm support for actually including that in the submitted report.

Also this appears to fail local mochitests:

370 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/test_hanging.html | This test left crash dumps behind, but we weren't expecting it to!
371 INFO TEST-INFO | Found unexpected crash dump file /tmp/tmpDEbAPo/minidumps/291ebbab-cb57-093b-21bcb2d1-5be7635a.dmp.

I spent a minute trying to figure out what was going on, but that goes into SpecialPowers code which is a maze of twisty passages, mostly-alike. Georg, use this patch as you will ;-)

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Submit multiple minidumps for plugin hangs

* Modified SpecialPowers code to 
  a) handle the new way of passing additional dumps and 
  b) not expect .extra files for every .dmp
* Updated the actual CrashSubmitter
* Fixed a test case

Try run: https://tbpl.mozilla.org/?tree=Try&rev=7e9883ed00b8

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Submit multiple minidumps for plugin hangs, v3

Above patch green on try except for unrelated oranges.
This patch just fixes inconsistent whitespace usage in it.

Comment 10

[Benjamin Smedberg]

Comment on attachment 658047 [details] [diff] [review]
Submit multiple minidumps for plugin hangs, v3

Tagging ted for review since gfritzsche and I both wrote parts of this patch.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 658047 [details] [diff] [review]
Submit multiple minidumps for plugin hangs, v3

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +304,5 @@
> +        crashReporter->AnnotateCrashReport(
> +            NS_LITERAL_CSTRING("additional_minidumps"),
> +            NS_LITERAL_CSTRING("browser"));
> +
> +        // TODO: collect Flash minidumps here

Are you going to handle this in a followup? If so you should file a bug and put the bug number here.

@@ +377,5 @@
>  
>      AnnotationTable notes;
>      notes.Init(4);
>      WriteExtraDataForMinidump(notes);
>          

Please kill the extraneous whitespace on this line while you're here.

::: dom/plugins/test/mochitest/hang_test.js
@@ +58,1 @@
>      

There are a few other lines with extraneous trailing whitespace. If your editor doesn't show it you can see it in the splinter view here.

::: testing/mochitest/tests/SimpleTest/SpecialPowersObserverAPI.js
@@ +52,5 @@
> +  }
> +  is.close();
> +  fstream.close();
> +  return parseKeyValuePairs(contents);
> +}

We should probably put these stupid functions in a .jsm at this point.

@@ +93,5 @@
>  
>    _getCrashDumpDir: function() {
>      if (!this._crashDumpDir) {
> +      var directoryService = Cc["@mozilla.org/file/directory_service;1"]
> +                             .getService(Ci.nsIProperties);

This file could probably be using Services.jsm as well, if you wanted to do that cleanup.

::: toolkit/crashreporter/CrashSubmit.jsm
@@ +237,5 @@
> +      
> +      for (let i of this.additionalDumps) {
> +        formData.append("upload_file_minidump_"+i.name, 
> +                        File(i.dump.path));
> +      }

Ostensibly you could just move this up into the first loop instead of iterating over the additional dumps twice, right? The order of the fields in the POST data probably isn't important. (Although that sort of depends on how the collector code was written.)

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +2535,5 @@
> +
> +  // turn "<id>.dmp" into "<id>-<name>.dmp
> +  leafName.Insert(NS_LITERAL_CSTRING("-") + name, leafName.Length() - 4);
> +
> +  minidump->MoveToNative(directory, leafName);

Native should be okay because the dump name will always be ASCII, right?

@@ +2560,5 @@
> +  return true;
> +}
> +
> +static bool
> +PairedDumpCallbackExtra(const XP_CHAR* dump_path,

I don't see where this method gets used.

::: toolkit/crashreporter/nsExceptionHandler.h
@@ +60,5 @@
>  bool GetExtraFileForMinidump(nsIFile* minidump, nsIFile** extraFile);
>  bool AppendExtraData(const nsAString& id, const AnnotationTable& data);
>  bool AppendExtraData(nsIFile* extraFile, const AnnotationTable& data);
> +void RenameAdditionalHangMinidump(nsIFile* minidump, nsIFile* childMinidump,
> +                                const nsACString& name);

Indentation seems wrong here.

Comment 12

[Georg Fritzsche [:gfritzsche]]

(In reply to Ted Mielczarek [:ted] from comment #11)
> ::: dom/plugins/ipc/PluginModuleParent.cpp
> @@ +304,5 @@
> > +        crashReporter->AnnotateCrashReport(
> > +            NS_LITERAL_CSTRING("additional_minidumps"),
> > +            NS_LITERAL_CSTRING("browser"));
> > +
> > +        // TODO: collect Flash minidumps here
> 
> Are you going to handle this in a followup? If so you should file a bug and
> put the bug number here.

That is bug 788512, which is blocked by this bug.
 
> ::: testing/mochitest/tests/SimpleTest/SpecialPowersObserverAPI.js
> @@ +52,5 @@
> > +  }
> > +  is.close();
> > +  fstream.close();
> > +  return parseKeyValuePairs(contents);
> > +}
> 
> We should probably put these stupid functions in a .jsm at this point.

Right, any suggestions on where this could go? I guess it would be better in a follow-up bug that changes all the places where those functions are being used?
 
> ::: toolkit/crashreporter/CrashSubmit.jsm
> @@ +237,5 @@
> > +      
> > +      for (let i of this.additionalDumps) {
> > +        formData.append("upload_file_minidump_"+i.name, 
> > +                        File(i.dump.path));
> > +      }
> 
> Ostensibly you could just move this up into the first loop instead of
> iterating over the additional dumps twice, right? The order of the fields in
> the POST data probably isn't important. (Although that sort of depends on
> how the collector code was written.)

Right, this shouldn't really matter.
 
> ::: toolkit/crashreporter/nsExceptionHandler.cpp
> @@ +2535,5 @@
> > +
> > +  // turn "<id>.dmp" into "<id>-<name>.dmp
> > +  leafName.Insert(NS_LITERAL_CSTRING("-") + name, leafName.Length() - 4);
> > +
> > +  minidump->MoveToNative(directory, leafName);
> 
> Native should be okay because the dump name will always be ASCII, right?

The dump names should always be ASCII (combination of dump id, '-' and ASCII names like "browser" and "flash1", "flash2", ...).

I'm going to put a revised patch up tomorrow.

Comment 13

[Benjamin Smedberg]

> 
> @@ +2560,5 @@
> > +  return true;
> > +}
> > +
> > +static bool
> > +PairedDumpCallbackExtra(const XP_CHAR* dump_path,
> 
> I don't see where this method gets used.

That's actually a bug! We should be using PairedDumpCallbackExtra the first time we call google_breakpad::ExceptionHandler::WriteMinidumpForChild (for the child minidump) but use PairedDumpCallback for the second time which writes the browser minidump, because we don't want an .extra file for that one.

Comment 14

[(not currently active) Ted Mielczarek]

(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> > We should probably put these stupid functions in a .jsm at this point.
> 
> Right, any suggestions on where this could go? I guess it would be better in
> a follow-up bug that changes all the places where those functions are being
> used?

I'd stick it right next to CrashSubmit.jsm in toolkit/crashreporter. If you feel like rolling that into this patch that'd be great, but if not please file a followup.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Attachment: Submit multiple minidumps for plugin hangs, v4

Try run: https://tbpl.mozilla.org/?tree=Try&rev=856a1ab012a0

Adressed review comments, fixed PairedDumpCallbackExtra usage.
Also rolled the parseKeyValuePairs/parseKeyValuePairsFromFile change into this patch as it was used in much fewer places than i thought (moved them to toolkit/crashreporter/KeyValueParser.jsm).

Ted, can i carry over the r+ on this?

Comment 16

[(not currently active) Ted Mielczarek]

Yes, that's fine.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

This is showing a few failures that look real to me (I starred the known-intermittent ones). I'll note that your last Try run showed the same OSX 10.8 M3 failure. Please review and address the failures before re-requesting checkin.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Attachment: 659503: Submit multiple minidumps for plugin hangs, v5

Fixed timing issue with skipping dom/plugins/test_crashing.html on OSX 10.8.
Try: https://tbpl.mozilla.org/?tree=Try&rev=4f62340a562f

Thanks for catching this Ryan, i completely overlooked it with all the linked bugs.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

What about the Win64 build failure?

Comment 20

[Georg Fritzsche [:gfritzsche]]

No idea yet, still looking into it.

Comment 21

[Georg Fritzsche [:gfritzsche]]

That Win64 build failure is gone, even though none of the patches should have touched it - possible build slave issue?

The rest looks good this time (checked with philor for the intermittent crash dumps being a known issue).

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Green on Try.

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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