[e10s] Generate several reports during the plugin hang

RESOLVED FIXED in Firefox 50

Status

()

Core
Plug-ins
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: blinky, Assigned: gsvelto)

Tracking

(Blocks: 1 bug, {regression})

50 Branch
mozilla50
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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
Flags: needinfo?(gsvelto)
(Assignee)

Comment 2

2 years ago
Thanks, this is precisely the information I needed, confirming an patch coming soon.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(gsvelto)

Updated

2 years ago
tracking-e10s: --- → ?
(Assignee)

Updated

2 years ago
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
Created attachment 8767771 [details] [diff] [review]
[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.
Attachment #8767771 - Flags: review?(jmathies)
(Assignee)

Comment 4

2 years ago
Created attachment 8767942 [details] [diff] [review]
[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
Attachment #8767771 - Attachment is obsolete: true
Attachment #8767771 - Flags: review?(jmathies)
Attachment #8767942 - Flags: review?(jmathies)

Updated

2 years ago
Attachment #8767942 - Flags: review?(jmathies) → review+

Comment 5

2 years ago
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

2 years ago
feel free to land the fix though, lets not hold that up.
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
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

2 years ago
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fecbe3e55c9
Do not take multiple minidumps during a hang r=jimm
(Assignee)

Updated

2 years ago
Blocks: 1284821

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1fecbe3e55c9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Reporter)

Updated

2 years ago
tracking-e10s: ? → ---
You need to log in before you can comment on or make changes to this bug.