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.

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.

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.

So bsmedberg says that this problem exists further back than bug 899347, so standing down.

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.

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).

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.

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.

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.

(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.

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?

This sounds reasonable, yes.

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.

/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

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.

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

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

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.

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

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.

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.

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.

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.

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

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

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

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

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

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.

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

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.

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

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

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)

(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.

(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.

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).

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

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

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

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

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

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).

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).

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. :)

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.

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/

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.

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2613087&repo=fx-team

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.

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

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