1284030 - [e10s] Generate several reports during the plugin hang

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[blinky]

Steps to reproduce:

1. Go to https://dl.dropboxusercontent.com/u/95157096/85f61cf7/fe80383b.swf.
2. When view the notification, do not kill plugin.
3. go to about:crashes.
4. Repeat the previous steps several times.
5. When view the notification, click on "Stop it".
6. Submit a crash report.

See https://dl.dropboxusercontent.com/u/95157096/85f61cf7/1kd0r3jozm.mp4


Actual results:

Generate several reports during the plugin hang.


Crash reports:

Note that only one report is marked 'hang'.

bp-20f8b0a0-3710-4926-88b4-be6962160703

bp-7465cad6-9e5e-4098-af9a-a06ad2160703
bp-53999e06-669a-47bc-8d9d-897402160703
bp-ee956a0e-8ac9-411e-b248-6dea82160703
bp-79b10316-8444-4e14-a969-553742160703

Comment 1

[blinky]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f9d32d5fc19a8cd5e408f7ff05283ff0c837061c&tochange=e309c8e399ad2bff9fe84993c52792fd688a5f81

Regressed by: bug 1262852

Comment 2

[Gabriele Svelto [:gsvelto]]

Thanks, this is precisely the information I needed, confirming an patch coming soon.

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Do not take multiple minidumps during a hang

This ensures that we don't take multiple minidumps from the same hang. It does so by storing the full minidump ID in the dump hash table replacing the browser ID so that we won't take a new one the next time we hit the same hang.

I've also done a couple of code hygiene adjustments:

- I've factored out the code taking the browser minidump; I've done so because this is thread-safe code and I wanted to highlight it

- Updating the minidump ID in the hash-table is similarly thread-safe so the two go hand in hand

- The dump ID and the hang data were always set and cleared together but had two separate functions to set them (SetHangData() and SetDumpId()). I've now consolidated both methods into a single one to make the intent clearer.

The try run is here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9966397db256

If this approach is not appropriate or if it doesn't work properly (the try run hasn't finished running yet) I'll back out the patch in bug 1262852 to buy us more time to fix this.

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Do not take multiple minidumps during a hang

The previous patch had an issue that prevented it from being built when the crash reporter was disabled; this is addressed in this patch the rest is unchanged. The try run is here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=838a2f792945

Comment 5

[Jim Mathies [:jimm]]

I wonder if there's some way we could write a test for this? We have a few plugin crash and hang tests under dom/plugins.

Comment 6

[Jim Mathies [:jimm]]

feel free to land the fix though, lets not hold that up.

Comment 7

[Gabriele Svelto [:gsvelto]]

(In reply to Jim Mathies [:jimm] from comment #5)
> I wonder if there's some way we could write a test for this? We have a few
> plugin crash and hang tests under dom/plugins.

It's definitely doable, by letting the process hang detector run for two full iterations and then checking how many dumps we leave behind. I'll file a follow up for it.

Comment 8

[Gabriele Svelto [:gsvelto]]

Thanks for the quick review Jim! I've re-triggered a few oranges in the try run and if everything turns green I'll land this quickly.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fecbe3e55c9
Do not take multiple minidumps during a hang r=jimm

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fecbe3e55c9