With e10s, plugin crash submit UI is broken

RESOLVED FIXED in Firefox 40

Status

()

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

People

(Reporter: Benjamin Smedberg, Assigned: mconley)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla40
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(e10sm6+, firefox40 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(13 attachments, 1 obsolete attachment)

404.74 KB, application/zip
Details
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
(Reporter)

Description

3 years ago
If a plugin crashes, the user doesn't have the option to submit the crash. Originally I thought that this was with e10s only, but I've tried it several times with non-e10s mode and the same problem persists.

STR:
* Launch a website which uses Flash, such as http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html
* Use crashfirefox.exe <pid-of-the-plugin-process>

EXPECTED:
The plugin crash UI should show a checkbox which includes "Send crash report"

ACTUAL:
The plugin crash UI shows, but it only says "The Adobe Flash plugin has crashed. Reload the page to try again." without any mention of submitting the crash report.

I don't know when this started: my initial guess would be all the way back to c07c6f96d613 2014-09-17 16:06 -0400 Mike Conley - Bug 899347 - [e10s] Make click-to-play work with e10s. Originally written by mbrubeck. r=gfritzsche,felipe,Unfocused. which means that this is broken in beta. I'll test that in a minute and if so this is an urgent problem to fix.
(Reporter)

Comment 1

3 years ago
I've verified that FF35.0b1 is affected.

[Tracking Requested - why for this release]: Users won't submit any plugin crashes, which could significantly harm stability metrics and diagnosis.
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → affected
tracking-firefox35: --- → ?
tracking-firefox36: --- → ?
tracking-firefox37: --- → ?

Updated

3 years ago
tracking-e10s: --- → ?

Updated

3 years ago
Blocks: 874016
(Assignee)

Comment 2

3 years ago
bsmedberg has confirmed that this was introduced in the Nightly after bug 899347 landed. I'm going to rollback the change locally to ensure that's the culprit.
(Assignee)

Comment 3

3 years ago
So bsmedberg says that this problem exists further back than bug 899347, so standing down.
(Reporter)

Comment 4

3 years ago
ok cancel red alert. My testcase wasn't showing the crash submission UI in non-e10s mode because the plugin was too small (200x200). That's a different bug that I'll file shortly.

I've altered the testcase so that it's 600x600, which shows that this works correctly in single-process mode but doesn't work in e10s mode. So this is a valid e10s bug but doesn't need to block the trains.
status-firefox35: affected → ---
status-firefox36: affected → ---
status-firefox37: affected → ---
tracking-firefox35: ? → ---
tracking-firefox36: ? → ---
tracking-firefox37: ? → ---
Keywords: regression
Summary: Plugin crash submit UI is broken → With e10s, plugin crash submit UI is broken

Updated

3 years ago
tracking-e10s: ? → m6+
Flags: firefox-backlog+
(Assignee)

Updated

3 years ago
Assignee: nobody → mconley
(Assignee)

Comment 5

3 years ago
So I know what's going wrong here. The "PluginCrashed" event, when fired in the content process, is missing the browser minidump id and the plugin minidump id. This is because the parent process is the only process that has access to that information at this time. Because that information is missing, PluginContent.jsm assumes there is no crash information and does not display the UI.

billm and I have a plan:

Somewhere inside where we set up the bridge with PluginModuleChromeParent and PluginModuleContentParent, create a unique identifier (I'll call it ID) for the plugin run. When the plugin crashes, have the PluginModuleContentParent::ActorDestroy send a synchronous message to the ContentParent asking for the dump information associated with the ID.

Here's where we might get a bit race-y.

When the PluginModuleChromeParent::ActorDestroy gets called, have it stash the browser and plugin dump ID information in a mapping somewhere.

_Hopefully_ we can guarantee that PluginModuleChromeParent::ActorDestroy gets called first. If that's the case, then the ContentParent can get the browser and plugin dump IDs from the mapping, and send those down to the child. The child can then pass those along to the plugin to dispatch the PluginCrashed event, and the in-content plugin crash UI should just work (it'll just send the dump IDs back up to the parent, and the parent will do the submitting work).
(Assignee)

Comment 6

3 years ago
To be clear the race isn't between the two ActorDestroy's - it's a race between the ActorDestroy in PluginModuleChromeParent and the ContentParent receiver of the sync message sent from the ActorDestroy of PluginModuleContentParent. I'll do some tests tomorrow to see what potential there is for races here.
(Assignee)

Comment 7

3 years ago
So I just ran a test where I set a breakpoint in a Recv message handler that I'd Send to from ActorDestroy in PluginModuleContentParent, and the ActorDestroy in PluginModuleChromeParent.

Over 10 tries, I wasn't able to ever have the Recv message handler get called first. If there's a race, I suspect it's far far more likely that PluginModuleChromeParent will have its ActorDestroy called first.

I think we're safe to move forward with this plan.
(Reporter)

Comment 8

3 years ago
I'm concerned about this plan. I feel like the content process should not have access to the crash ID, and the presentation changes necessary for crash submission in the Firefox UI should be driven by the chrome process.

Discussing how frequent a race might occur seems odd; if we're going to rely on a certain event ordering, we should be able to prove that it will be delivered this way. The order that processes receive pipe error notifications from the OS seems inherently racy. I don't see how you can reason about this ordering given the setup here to build a reliable system.
(Assignee)

Comment 9

3 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> I'm concerned about this plan. I feel like the content process should not
> have access to the crash ID, and the presentation changes necessary for
> crash submission in the Firefox UI should be driven by the chrome process.
> 

Alright - I can draw up a plan for altering how the in-content plugin crash UI works, and we can compare.

> Discussing how frequent a race might occur seems odd; if we're going to rely
> on a certain event ordering, we should be able to prove that it will be
> delivered this way. The order that processes receive pipe error
> notifications from the OS seems inherently racy. I don't see how you can
> reason about this ordering given the setup here to build a reliable system.

I didn't really know how else to test for the race that billm was concerned about - especially given my lack of experience with under-the-hood IPC traffic (the traffic the IPC library relies on to do things like set up and tear down of actors). I had no idea that a pipe error caused the ActorDestroy in the plugin crash case, but now that you mention it, it makes sense - and you're right, that immediately puts my analysis out to pasture.
(Assignee)

Comment 10

3 years ago
Ok, I think I've come up with an alternative.

Similar to my last solution, I propose that every running instance of a plugin gets a unique plugin run ID, so that we can differentiate plugin runs (this is to account for the possibility of a crash occurring for a plugin of type X just before another plugin of type X is being instantiated - the two instances of the same plugin will get different run IDs).

When the ActorDestroy method is called in PluginModuleChromeParent, we process the minidump, and then we send either a chrome-only event or an observer notification saying that the plugin with some run ID has crashed.

browser-plugins.js code will then capture the observer notification or event, and then send a message down to each browser in that window saying that a plugin with some run ID has crashed, and whether or not a crash report can be made. It's possible I can shortcut this by using the global message manager to tell every listener everywhere about it. I'm not too worried about the details there.

PluginContent.jsm's PluginContent will hear the message for each browser, and then search the DOM for plugins that match that run ID. When it finds them, it'll put them in their crashed UI state, with a crash report UI if the message said we can send off a report. In the event that the the region that the plugin renders in is too small, we'll do what we currently do, and send a message back up to the parent to show a notification bar.

If and when a crash report is sent, another message will be sent down to all browsers saying so, and that'll clear out the crash report UI for each crashed plugin.

We can possibly then deprecate the PluginCrashed event, as we will no longer be using it.

Does that sound like a more appropriate solution, bsmedberg?
Flags: needinfo?(benjamin)
(Reporter)

Comment 11

3 years ago
This sounds reasonable, yes.
Flags: needinfo?(benjamin)
(Assignee)

Comment 12

3 years ago
So I've been trying to figure out the best way of getting the PluginModuleChromeParent and the PluginModuleContentParent sync'd up on plugin run IDs.

:handyman suggested I just use the process ID as the run ID, but I'm reluctant to expose that sort of low-level system information to chrome-level through nsIObjectLoadingContent. Is that reluctance unfounded?

Assuming it isn't, here's my plan on getting run IDs passed around.

PluginModuleChromeParent will be in charge of sending out run IDs - there'll be some simple static uint counter that it increments every time a new PluginModuleChromeParent gets created, which PluginModuleChromeParent will stash in some private member variable.

PluginModuleChromeParent will then, eventually, call AssociateWithProcessId in the multi-process case. This messages the ContentChild, passing along the plugin ID and the plugin process ID. I propose passing along the run ID here as well.

In the ContentChild, the plugin ID and plugin process ID get stashed into a PluginModuleMapping. We should stash thee run ID in there as well.

Eventually, through ContentChild::AllocPPluginModuleParent, PluginModuleContentParent::Initialize will get called. This grabs the PluginModuleMapping out of the linked list (it'll be removed from the list as soon as this method exits), then we should call something like parent->SetRunID passing along the run ID that had been stashed in the PluginModuleMapping.

That should put PluginModuleChromeParent and PluginModuleContentParent on even keel in terms of run IDs.

That way, we can expose the run ID through nsIObjectLoadingContent by way of nsPluginInstanceOwner -> PluginModule(Chrome|Content)Parent, which the front-end can then use to switch plugins to the crashed UI state, as per comment 10.
(Assignee)

Comment 13

3 years ago
Created attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5703 - Bug 1110887 - Add a run ID for plugins to differentiate subsequent runs of the same plugins. r=?
/r/5705 - Bug 1110887 - Expose run ID through nsIObjectLoadingContent.idl. r=?
/r/5707 - Bug 1110887 - Send the run ID and plugin name along with the plugin-crashed observer notification. r=?
/r/5709 - [WIP] Bug 1110887 - Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?

Pull down these commits:

hg pull review -r 5ace3d8b124edcdb798af0350cea74c8018958b1
(Assignee)

Updated

2 years ago
See Also: → bug 1146955
(Assignee)

Comment 14

2 years ago
I'm pretty certain I can get the NPAPI crash case working independent from the GMP crash case (which is probably preferable, otherwise this patch series will get rather unruly). I've spun out the GMP crash case to its own bug - bug 1146955.
(Assignee)

Comment 15

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5705 - Bug 1110887 - Expose run ID through nsIObjectLoadingContent.idl. r=?
/r/5707 - Bug 1110887 - Send the run ID and plugin name along with the plugin-crashed observer notification. r=?
/r/5709 - [WIP] Bug 1110887 - Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - [WIP] Bug 1110887 - Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - [WIP] Bug 1110887 - Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - [WIP] Bug 1110887 - If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - [WIP] Bug 1110887 - Make the plugin crashed notification bar work with run IDs. r=?

Pull down these commits:

hg pull review -r 2b73fb87996e5cb637ec35f9da1a722c90d91c23
(Assignee)

Comment 16

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/6101 - Bug 1110887 - Add a run ID for plugins to differentiate subsequent runs of the same plugins. r=?
/r/5705 - Bug 1110887 - Expose run ID through nsIObjectLoadingContent.idl. r=?
/r/5707 - Bug 1110887 - Send the run ID and plugin name along with the plugin-crashed observer notification. r=?
/r/5709 - [WIP] Bug 1110887 - Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - [WIP] Bug 1110887 - Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - [WIP] Bug 1110887 - Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - [WIP] Bug 1110887 - If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - [WIP] Bug 1110887 - Make the plugin crashed notification bar work with run IDs. r=?

Pull down these commits:

hg pull review -r dada070d9b37fa0cda51c13b6c392e1c87df60fc
(Assignee)

Comment 17

2 years ago
https://reviewboard.mozilla.org/r/6101/#review5091

::: dom/plugins/ipc/PluginModuleParent.cpp
(Diff revision 1)
>      PluginModuleChromeParent::ClearInstantiationFlag();

Should we be null-checking rv and runID here?

::: dom/plugins/ipc/PluginModuleParent.cpp
(Diff revision 1)
> +    NS_ENSURE_ARG_POINTER(aRunID);

I don't believe we're supposed to use `NS_ENSURE_*` anymore, according to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros

::: dom/plugins/ipc/PluginModuleParent.cpp
(Diff revision 1)
> +    mRunID = sNextRunID++;

In the failure case, I'm defaulting run ID to 0. Maybe we can make that be the "failure" value - like, have the initial run ID be 1. If it's 0, something's gone wrong.
(Assignee)

Comment 18

2 years ago
https://reviewboard.mozilla.org/r/5705/#review5093

::: dom/base/nsObjectLoadingContent.cpp
(Diff revision 3)
> +    NS_ENSURE_SUCCESS(rv, rv);

Don't use NS_ENSURE_SUCCESS as per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros

::: dom/base/nsObjectLoadingContent.cpp
(Diff revision 3)
> +  NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome(), NS_ERROR_NOT_AVAILABLE);
> +  NS_ENSURE_ARG_POINTER(aRunID);

Do not use NS_ENSURE_*, as per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
(Diff revision 3)
> +  NS_ENSURE_ARG_POINTER(aRunID);

Do not use NS_ENSURE_*: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros
(Assignee)

Comment 19

2 years ago
https://reviewboard.mozilla.org/r/5707/#review5095

::: dom/plugins/base/nsPluginHost.cpp
(Diff revision 3)
> +    nsresult rv = aPlugin->GetLibrary()->GetRunID(&runID);

We should probably ensure that GetLibrary() returns us something.
(Assignee)

Comment 20

2 years ago
https://reviewboard.mozilla.org/r/5709/#review5119

::: browser/base/content/browser-plugins.js
(Diff revision 3)
> +      throw new Error("FIX ME");

Replace this with something sensible.

::: browser/base/content/browser-plugins.js
(Diff revision 3)
>    pluginCrashed : function(subject, topic, data) {

Should this be NPAPIPluginCrashed, if we're differentiating now?

::: browser/base/content/browser-plugins.js
(Diff revision 3)
> +    if (AppConstants.MOZ_CRASHREPORTER) {
> -    let pluginDumpID = propertyBag.getPropertyAsAString("pluginDumpID");
> +      let pluginDumpID = propertyBag.getPropertyAsAString("pluginDumpID");
> -    let browserDumpID= propertyBag.getPropertyAsAString("browserDumpID");
> -    let shouldSubmit = gCrashReporter.submitReports;
> -    let doPrompt     = true; // XXX followup to get via gCrashReporter
> +      // If we don't have a minidumpID, we can't (or didn't) submit anything.
> +      // This can happen if the plugin is killed from the task manager.
> +      hasReport = !!pluginDumpID;
> -
> -    // Submit automatically when appropriate.
> -    if (pluginDumpID && shouldSubmit && !doPrompt) {
> -      this.submitReport(pluginDumpID, browserDumpID);
> -      // Submission is async, so we can't easily show failure UI.
> -      propertyBag.setPropertyAsBool("submittedCrashReport", true);
>      }

This can be simplified.

```
let pluginDumpID = propertyBag.getPropertyAsAString("pluginDumpID");
let hasReport = AppConstants.MOZ_CRASHREPORTER && !!pluginDumpID;

let state = hasReport ? "please" : "noReport";
//...
```

::: browser/modules/PluginContent.jsm
(Diff revision 3)
>      this.pluginData = new Map();
> +    // Cache of plugin crash information sent from the parent
> +    this.pluginCrashData = new Map();

Maybe rename this to NPAPIPluginData and NPAPIPluginCrashData in a follow-up?

::: browser/modules/PluginContent.jsm
(Diff revision 3)
> -  // Crashed-plugin event listener. Called for every instance of a
> +  onPluginCrashed: function (target, aEvent) {

We should add a comment here that the PluginCrashed can be fired both for NPAPI and Gecko Media plugins.
(Assignee)

Comment 21

2 years ago
https://reviewboard.mozilla.org/r/5703/#review5121

::: browser/modules/ContentCrashReporters.jsm
(Diff revision 3)
> +      throw new Error("FIX ME");

Do something sensible here.

::: browser/modules/ContentCrashReporters.jsm
(Diff revision 3)
> +      // TODO: Do something smart?

Reporting the error might be smart enough.
(Assignee)

Comment 22

2 years ago
https://reviewboard.mozilla.org/r/6061/#review5123

::: browser/modules/PluginContent.jsm
(Diff revision 2)
> +      // TODO: Do something smart.

Fix this.

::: browser/modules/PluginContent.jsm
(Diff revision 2)
> +      // TODO: Do something smart.

Fix this.
(Assignee)

Comment 23

2 years ago
https://reviewboard.mozilla.org/r/6063/#review5125

::: browser/modules/PluginContent.jsm
(Diff revision 2)
> +          runId: msg.data.runId,

runId

::: browser/modules/PluginContent.jsm
(Diff revision 2)
> +  NPAPIPluginCrashReportSubmitted: function({ runId, state }) {

runId

::: browser/modules/PluginContent.jsm
(Diff revision 2)
> +    this.pluginCrashData.delete(runId);

runId

::: browser/modules/PluginContent.jsm
(Diff revision 2)
> +          plugin.runId == runId) {

runId
(Assignee)

Updated

2 years ago
Depends on: 1148012
(Assignee)

Comment 24

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?

Pull down these commits:

hg pull review -r ca5d4cddde852b9c32f2750610e9ff3132848f2a
(Assignee)

Comment 25

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6181 - [WIP] Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?

Pull down these commits:

hg pull review -r ccab6d762f03ff647ff811744026f5a205cb5523
(Assignee)

Comment 26

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6181 - Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?

Pull down these commits:

hg pull review -r 2578df4ae210455ec4472ba591b10f58722f8f4e
(Assignee)

Updated

2 years ago
Attachment #8580048 - Flags: review?(dtownsend)
(Assignee)

Comment 27

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6181 - Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?

Pull down these commits:

hg pull review -r 2578df4ae210455ec4472ba591b10f58722f8f4e
(Assignee)

Comment 28

2 years ago
Hey Mossop - I'm not 100% ready for parts 1-5 to be reviewed, but I think the test that I added can be looked at.

Testing this non-determinism stuff was pretty challenging - and I found a way of doing it, but let me know if you can think of an easier, simpler or more direct way.
(Assignee)

Comment 29

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6181 - Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?

Pull down these commits:

hg pull review -r 2578df4ae210455ec4472ba591b10f58722f8f4e
Attachment #8580048 - Flags: review?(felipc)
(Assignee)

Comment 30

2 years ago
Hey Felipe - so, the patches I just flagged you for review on are not really atomic. Like, I'm pretty sure they'd break some plugin tests if they aren't all applied. I broke them apart like this though because I figure it makes for easier reviewing.

I think I'm planning on folding them all together before landing though.
(Assignee)

Updated

2 years ago
Blocks: 1148541
(Assignee)

Comment 31

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6181 - Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?

Pull down these commits:

hg pull review -r d5432f1bfd22ccfb4f411f48d73b9b856cfe5343
(Assignee)

Comment 32

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6181 - Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?

Pull down these commits:

hg pull review -r d5432f1bfd22ccfb4f411f48d73b9b856cfe5343
Attachment #8580048 - Flags: review?(dtownsend)
https://reviewboard.mozilla.org/r/6181/#review5223

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
(Diff revision 2)
> +    }

I think this needs to pass remote or non-remote depending on the value of options.remote. Basically if options.remote is undefined the new window should have the same state, otherwise it should have the state requested.

::: browser/base/content/test/plugins/browser_pluginCrashReportNonDeterminism.js
(Diff revision 2)
> +      // anything like waitForCondition, so we have to roll our own.

Add one to a jsm (i.e. make a start on bug 1144797)
(Assignee)

Updated

2 years ago
Depends on: 1144797
(Assignee)

Comment 34

2 years ago
(In reply to Dave Townsend [:mossop] from comment #33)
> https://reviewboard.mozilla.org/r/6181/#review5223
> 
> ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
> (Diff revision 2)
> > +    }
> 
> I think this needs to pass remote or non-remote depending on the value of
> options.remote. Basically if options.remote is undefined the new window
> should have the same state, otherwise it should have the state requested.
> 

I'm not sure I understand. "non-remote" is, as far as I can tell, not something we can pass in the feature string when opening a new window. Are you saying that new windows should default to the remoteness of their opener? That seems to go against what we do with options.private... or am I misunderstanding here?


> :::
> browser/base/content/test/plugins/browser_pluginCrashReportNonDeterminism.js
> (Diff revision 2)
> > +      // anything like waitForCondition, so we have to roll our own.
> 
> Add one to a jsm (i.e. make a start on bug 1144797)

Ok - spun out to that bug.
(Assignee)

Updated

2 years ago
Flags: needinfo?(dtownsend)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #34)
> (In reply to Dave Townsend [:mossop] from comment #33)
> > https://reviewboard.mozilla.org/r/6181/#review5223
> > 
> > ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
> > (Diff revision 2)
> > > +    }
> > 
> > I think this needs to pass remote or non-remote depending on the value of
> > options.remote. Basically if options.remote is undefined the new window
> > should have the same state, otherwise it should have the state requested.
> > 
> 
> I'm not sure I understand. "non-remote" is, as far as I can tell, not
> something we can pass in the feature string when opening a new window. Are
> you saying that new windows should default to the remoteness of their
> opener? That seems to go against what we do with options.private... or am I
> misunderstanding here?

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3816 says that non-remote is an option. It's how we open non-e10s windows.

Disregard about it inheriting, I just want if you don't pass the remote options at all it should do whatever Nightly does when you do "File - Open Window" which seems to always be remote if remote is on regardless of the opener.
Flags: needinfo?(dtownsend)
(Assignee)

Comment 36

2 years ago
https://reviewboard.mozilla.org/r/6181/#review5241

> I think this needs to pass remote or non-remote depending on the value of options.remote. Basically if options.remote is undefined the new window should have the same state, otherwise it should have the state requested.

Ah, ok. It seems that by default, openNewBrowserWindow will always open a new window with the profile default remoteness state. I've made it so that remote can be set to true (to force remote), false (to force non-remote), and omitted (to go with default).
(Assignee)

Comment 37

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6247 - Bug 1110887 - Part 6: Remove event and message listeners in PluginContent when uninitting. r=?
/r/6181 - Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?
/r/6249 - Bug 1110887 - Update browser_CTP_crashreporting.js to work with e10s changes. r=?
/r/6251 - Bug 1110887 - Update browser_globalplugin_crashinfobar.js to work with e10s changes. r=?

Pull down these commits:

hg pull review -r c42cac354645b455e09a9c23a40287d54f210588
(Assignee)

Comment 38

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6247 - Bug 1110887 - Part 6: Remove event and message listeners in PluginContent when uninitting. r=?
/r/6181 - Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?
/r/6249 - Bug 1110887 - Update browser_CTP_crashreporting.js to work with e10s changes. r=?
/r/6251 - Bug 1110887 - Update browser_globalplugin_crashinfobar.js to work with e10s changes. r=?

Pull down these commits:

hg pull review -r 72e1e0d8b7ad5eae76b295da6dea2b3f338bf8a2
(Assignee)

Comment 39

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6247 - Bug 1110887 - Part 6: Remove event and message listeners in PluginContent when uninitting. r=?
/r/6181 - Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?
/r/6249 - Bug 1110887 - Update browser_CTP_crashreporting.js to work with e10s changes. r=?
/r/6251 - Bug 1110887 - Update browser_globalplugin_crashinfobar.js to work with e10s changes. r=?

Pull down these commits:

hg pull review -r f69445b9197bbe53d753d50932a5391796a51cf2
(Assignee)

Comment 40

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6247 - Bug 1110887 - Part 6: Remove event and message listeners in PluginContent when uninitting. r=?
/r/6181 - Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?
/r/6249 - Bug 1110887 - Update browser_CTP_crashreporting.js to work with e10s changes. r=?
/r/6251 - Bug 1110887 - Update browser_globalplugin_crashinfobar.js to work with e10s changes. r=?

Pull down these commits:

hg pull review -r e039b82151f44a2485d4f1aa47d0265d8db36776
(Assignee)

Comment 41

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=?
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=?
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6247 - Bug 1110887 - Part 6: Remove event and message listeners in PluginContent when uninitting. r=?
/r/6181 - Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?
/r/6249 - Bug 1110887 - Update browser_CTP_crashreporting.js to work with e10s changes. r=?
/r/6251 - Bug 1110887 - Update browser_globalplugin_crashinfobar.js to work with e10s changes. r=?

Pull down these commits:

hg pull review -r 8d904867801c54246cd713403ddcd41c4fedd9c8
(Assignee)

Comment 42

2 years ago
https://reviewboard.mozilla.org/r/5709/#review5589

> a #ifdef MOZ_CRASHREPORTER wrapper was removed in this part of the code. Was that intetional?

I'm using AppConstands.MOZ_CRASHREPORTER instead on line 476. But as you helped me realize yesterday, I also need to pass the "noSubmit" state down if the crash reporter is disabled.

> i'm not sure if it's some ReviewBoard confusion on my part (looking at the root review which shows all diffs), but I can't seem to find where this part about submitting automatically when appropriate went

According to bsmedberg, it was never possible to submit crash reports automatically from plugin crashes - though that might have been a goal at one point. So I think we can safely remove that stuff (I'll file a followup bug to remove "submittedCrashReport" from the PluginCrashed event entirely).
(Assignee)

Comment 43

2 years ago
https://reviewboard.mozilla.org/r/6063/#review5591

> i'm not sure if this is fine here, or if .delete should only be called when the promise resolves. Any thoughts?

I don't think there's much value in keeping it around unless we want to have CrashSubmit re-try sending the crash report. Even then - CrashSubmit has all of the information it needs to retry sending the report, so I don't think there's any other component that's going to want those crash dump IDs after the fact.

The retry-sending thing for CrashSubmit is a good idea for follow-up work (which I will file).
(Assignee)

Comment 44

2 years ago
https://reviewboard.mozilla.org/r/6065/#review5593

> why the change from the #ifdef to the runtime check?

We discussed this in IRC yesterday, but for the record, this is because of the conversation some folks have been having on the firefox-dev mailing list about moving away from build-time ifdefs / defines in our Javascript, to using AppConstants. This will, hopefully, allow us to use better JS tooling in the future.

> nit: looks like some tabs where introduced here?

Those little >> symbols are Review Board's way of telling you that the indentation changed but that the line, itself, did not otherwise change. So these aren't tabs, they're just spaces. Tabs are for weirdos. :)
(Assignee)

Comment 45

2 years ago
https://reviewboard.mozilla.org/r/6247/#review5595

I was seeing some warnings / errors in the console when running tests for messages being received after PluginContent was supposed to have been torn down. No leaks, but I wasn't testing for any. I don't think it makes much sense to keep these around beyond the lifetime of the page, which is why I decided to get rid of them.
(Assignee)

Comment 46

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

/r/5709 - Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
/r/5703 - Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=felipe.
/r/6061 - Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=felipe.
/r/6063 - Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
/r/6065 - Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
/r/6247 - Bug 1110887 - Part 6: Remove event and message listeners in PluginContent when uninitting. r=?
/r/6181 - Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?
/r/6249 - Bug 1110887 - Update browser_CTP_crashreporting.js to work with e10s changes. r=?
/r/6251 - Bug 1110887 - Update browser_globalplugin_crashinfobar.js to work with e10s changes. r=?

Pull down these commits:

hg pull -r 9d4cdbcbb3624a727ac30b9b1eadf10cc7c4be9e https://reviewboard-hg.mozilla.org/gecko/
(Assignee)

Comment 47

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)

r+'d by felipe here:

https://reviewboard.mozilla.org/r/5701/

but Bugzilla didn't get updated because felipe's Bugzilla cookie expired.


Thanks felipe!  Landed as:

remote:   https://hg.mozilla.org/integration/fx-team/rev/503877a91767
remote:   https://hg.mozilla.org/integration/fx-team/rev/71310d7e7e2c
remote:   https://hg.mozilla.org/integration/fx-team/rev/d57993c7e315
remote:   https://hg.mozilla.org/integration/fx-team/rev/238352c73015

I have a number of follow-ups for various clean-uppy things that I will file tomorrow.
Attachment #8580048 - Attachment description: MozReview Request: bz://1110887/mconley → MozReview Request: bz://1110887/mconley (r+'d by felipe)
Attachment #8580048 - Flags: review?(felipc) → review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2613087&repo=fx-team

Updated

2 years ago
Flags: needinfo?(mconley)
(Assignee)

Comment 49

2 years ago
It looks like the content process is crashing...

Here's a screenshot of the error occurring from the test machine:

http://i.imgur.com/Tf7xbfb.png

This seems to be the most interesting part of the log:

[Parent 22296] WARNING: '!aObserver', file /media/Projects/mozilla/mozilla-central/xpcom/ds/nsObserverService.cpp, line 287
[Child 22926] ###!!! ABORT: X_CreateGC: BadDrawable (invalid Pixmap or Window parameter); sync; id=0x4400008: file /media/Projects/mozilla/mozilla-central/toolkit/xre/nsX11ErrorHandler.cpp, line 157
#01: X11Error (/media/Projects/mozilla/mozilla-central/toolkit/xre/nsX11ErrorHandler.cpp:158)
[Child 22926] ###!!! ABORT: X_CreateGC: BadDrawable (invalid Pixmap or Window parameter); sync; id=0x4400008: file /media/Projects/mozilla/mozilla-central/toolkit/xre/nsX11ErrorHandler.cpp, line 157
Hit MOZ_CRASH() at /media/Projects/mozilla/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:33

So we're aborting because of a bad drawable, it seems.

I'm going to disable this test for Linux for now, and file a follow-up to investigate and re-enable.
Flags: needinfo?(mconley)
(Assignee)

Updated

2 years ago
Depends on: 1152811
(Assignee)

Comment 50

2 years ago
Disabled the test for Linux (and filed bug 1152811 to investigate and re-open).

Re-landed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/91df06a7b643
remote:   https://hg.mozilla.org/integration/fx-team/rev/d3fb698e4f93
remote:   https://hg.mozilla.org/integration/fx-team/rev/db237a68215f
remote:   https://hg.mozilla.org/integration/fx-team/rev/c29695f586bb
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/91df06a7b643
https://hg.mozilla.org/mozilla-central/rev/d3fb698e4f93
https://hg.mozilla.org/mozilla-central/rev/db237a68215f
https://hg.mozilla.org/mozilla-central/rev/c29695f586bb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Updated

2 years ago
Iteration: --- → 40.1 - 13 Apr
Depends on: 1153659
(Assignee)

Comment 52

2 years ago
bugnotes
Created attachment 8591652 [details]
Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1110887.html
(Assignee)

Updated

2 years ago
Depends on: 1153894
(Assignee)

Comment 53

2 years ago
Comment on attachment 8580048 [details]
MozReview Request: bz://1110887/mconley (r+'d by felipe)
Attachment #8580048 - Attachment is obsolete: true
Attachment #8618904 - Flags: review+
Attachment #8618905 - Flags: review+
Attachment #8618906 - Flags: review+
Attachment #8618907 - Flags: review+
Attachment #8618908 - Flags: review+
Attachment #8618909 - Flags: review+
Attachment #8618910 - Flags: review+
Attachment #8618911 - Flags: review+
Attachment #8618912 - Flags: review+
Attachment #8618913 - Flags: review+
Attachment #8618914 - Flags: review+
Attachment #8618915 - Flags: review+
(Assignee)

Comment 54

2 years ago
Created attachment 8618904 [details]
MozReview Request: Bug 1110887 - Send the run ID and plugin name along with the plugin-crashed observer notification. r=?
(Assignee)

Comment 55

2 years ago
Created attachment 8618905 [details]
MozReview Request: Bug 1110887 - Add test to exercise both cases of some non-determinism when dealing with plugin crashes in e10s. r=?
(Assignee)

Comment 56

2 years ago
Created attachment 8618906 [details]
MozReview Request: Bug 1110887 - Part 6: Remove event and message listeners in PluginContent when uninitting. r=?
(Assignee)

Comment 57

2 years ago
Created attachment 8618907 [details]
MozReview Request: Bug 1110887 - Update browser_CTP_crashreporting.js to work with e10s changes. r=?
(Assignee)

Comment 58

2 years ago
Created attachment 8618908 [details]
MozReview Request: Bug 1110887 - Part 2: Introduce PluginCrashReporter, and rename TabCrashReporter.jsm to ContentCrashReporters.jsm. r=felipe.
(Assignee)

Comment 59

2 years ago
Created attachment 8618909 [details]
MozReview Request: Bug 1110887 - Update browser_globalplugin_crashinfobar.js to work with e10s changes. r=?
(Assignee)

Comment 60

2 years ago
Created attachment 8618910 [details]
MozReview Request: Bug 1110887 - Part 3: Make gPluginHandler use PluginCrashReporter to submit plugin crashes. r=felipe.
(Assignee)

Comment 61

2 years ago
Created attachment 8618911 [details]
MozReview Request: Bug 1110887 - Part 4: If a plugin crash report was submitted, put all visible instances into the submitted state. r=?
(Assignee)

Comment 62

2 years ago
Created attachment 8618912 [details]
MozReview Request: Bug 1110887 - Part 5: Make the plugin crashed notification bar work with run IDs. r=?
(Assignee)

Comment 63

2 years ago
Created attachment 8618913 [details]
MozReview Request: Bug 1110887 - Add a run ID for plugins to differentiate subsequent runs of the same plugins. r=?
(Assignee)

Comment 64

2 years ago
Created attachment 8618914 [details]
MozReview Request: Bug 1110887 - Expose run ID through nsIObjectLoadingContent.idl. r=?
(Assignee)

Comment 65

2 years ago
Created attachment 8618915 [details]
MozReview Request: Bug 1110887 - Part 1: Have the gPluginHandler tell content processes when a NPAPI plugin crashes. r=?
You need to log in before you can comment on or make changes to this bug.