Closed Bug 1235056 Opened 8 years ago Closed 8 years ago

Intermittent e10s browser_pluginCrashCommentAndURL.js | The crash UI should be visible - false == true - | Test timed out

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox45 --- wontfix
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed
firefox-esr45 --- unaffected

People

(Reporter: aryx, Assigned: bytesized)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #1160780 +++

https://treeherder.mozilla.org/logviewer.html#?job_id=19000369&repo=mozilla-inbound

05:31:32     INFO -  53 INFO TEST-START | browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js
05:31:32     INFO -  XPCOM_MEM_BLOAT_LOG: /var/folders/qg/7pj314vs6ns665sg_7g6q8x000000w/T/tmpU1vGYw.mozrunner/runtests_leaks.log
05:31:32     INFO -  Writing to log: /var/folders/qg/7pj314vs6ns665sg_7g6q8x000000w/T/tmpU1vGYw.mozrunner/runtests_leaks_plugin_pid1721.log
05:31:33     INFO -  ###!!! [Parent][MessageChannel::Call] Error: Channel error: cannot send/recv
05:31:33     INFO -  ###!!! [Parent][MessageChannel::Call] Error: (msgtype=0xAA0014,name=PPluginInstance::Msg_NPP_HandleEvent) Channel error: cannot send/recv
05:33:02     INFO -  TEST-INFO | started process screencapture
05:33:02     INFO -  TEST-INFO | screencapture: exit 0
05:33:02     INFO -  54 INFO checking window state
05:33:02     INFO -  55 INFO Entering test
05:33:02     INFO -  56 INFO Leaving test
05:33:02     INFO -  57 INFO Entering test
05:33:02     INFO -  58 INFO Wait tab event: load
05:33:02     INFO -  59 INFO Tab event received: load
05:33:02     INFO -  60 INFO Console message: [JavaScript Error: "1450963927461	Toolkit.GMP	ERROR	GMPInstallManager.simpleCheckAndInstall Could not check for addons: Error: got node name: html, expected: updates (resource://gre/modules/addons/ProductAddonChecker.jsm:145:1) JS Stack trace: parseXML@ProductAddonChecker.jsm:145:1 < promise callback*ProductAddonChecker.getProductAddonList@ProductAddonChecker.jsm:299:12 < GMPInstallManager.prototype.checkForAddons@GMPInstallManager.jsm:107:5 < GMPInstallManager.prototype.simpleCheckAndInstall<@GMPInstallManager.jsm:204:29 < gBrowserInit._delayedStartup/<@browser.js:1359:7 < setTimeout handler*gBrowserInit._delayedStartup@browser.js:1355:1 < EventListener.handleEvent*gBrowserInit.onLoad@browser.js:1073:5 < onload@browser.xul:1:1" {file: "resource://gre/modules/Log.jsm" line: 751}]
05:33:02     INFO -  61 INFO Console message: 1450963927500	Services.HealthReport.HealthReporter	WARN	Saved state file does not exist.
05:33:02     INFO -  62 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js | Test timed out -
05:33:02     INFO -  MEMORY STAT | vsize 3523MB | residentFast 483MB | heapAllocated 102MB
05:33:02     INFO -  63 INFO TEST-OK | browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js | took 90070ms
05:33:02     INFO -  64 INFO TEST-START | browser/base/content/test/plugins/browser_pluginCrashReportNonDeterminism.js
Blocks: e10s-tests
tracking-e10s: --- → +
Attached patch Patch - add logging (obsolete) — Splinter Review
The error message here looks like an update check it running when it shouldn't, and getting an HTML document in response when it shouldn't. However, that could also just be a red herring. Either way, I'm hoping enabling some extra logging here will help. (I can't reproduce locally, and it occurs rare enough that best bet isn't Try)
Attachment #8712452 - Flags: review?(dtownsend)
I rewrote this test a few months ago and have been monitoring the error logs.  All of the test failures for this test contain the failed GMP update check message in the logs as Blair has pointed out.  Successful runs do not seem to contain the same message about the GMP update check failing.  I also noticed that every time this test fails, if you scroll up a few lines in the log, you will find the following line:

05:31:31 INFO - JavaScript error: resource:///modules/PluginContent.jsm, line 830: TypeError: this.global is undefined

this.global[1] is supposed to hold a reference to the content window so if it is undefined it would make sense that we never get a PluginCrashed event since we listen for that event on the content window.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#34
Comment on attachment 8712452 [details] [diff] [review]
Patch - add logging

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

I'd expect the GMP update URL to be pointing to a dummy URL and so probably getting a 404 document back.
Attachment #8712452 - Flags: review?(dtownsend) → review+
(In reply to Trevor Rowbotham from comment #5)
> Successful runs do not
> seem to contain the same message about the GMP update check failing.

Was curious about that too - I couldn't find any case of that either, but manually looking through successful logs feels too much like anecdotal evidence :\

>  I also
> noticed that every time this test fails, if you scroll up a few lines in the
> log, you will find the following line:
> 
> 05:31:31 INFO - JavaScript error: resource:///modules/PluginContent.jsm,
> line 830: TypeError: this.global is undefined

Yea.. I can't tell if that's related to the above or not :\

And the automated screenshots shows are showing the page is still there.

(In reply to Dave Townsend [:mossop] from comment #6)
> I'd expect the GMP update URL to be pointing to a dummy URL and so probably
> getting a 404 document back.

Hopefully :)
I see the GMP update check all the time, because I only look at logs for failed tests. I assume it tries to run when we think the user has gone off to get a beer, a condition that's also matched by a test sitting idle.
(In reply to Phil Ringnalda (:philor) from comment #9)
> I see the GMP update check all the time, because I only look at logs for
> failed tests. I assume it tries to run when we think the user has gone off
> to get a beer, a condition that's also matched by a test sitting idle.

This is what I wondered :) Thanks for that info. It shouldn't be running... but that also means it likely isn't related to this particular bug. Filed bug 1243690.
https://hg.mozilla.org/mozilla-central/rev/0ecd7d72f232
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla47 → ---
Got the log I wanted:

> 07:03:39     INFO -  71 INFO TEST-START | browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js
> 07:03:40     INFO -  XPCOM_MEM_BLOAT_LOG: /tmp/tmpBoKVC_.mozrunner/runtests_leaks.log
> 07:03:40     INFO -  Writing to log: /tmp/tmpBoKVC_.mozrunner/runtests_leaks_plugin_pid2197.log
> 07:03:40     INFO -  ###!!! [Parent][MessageChannel::Call] Error: Channel error: cannot send/recv
> 07:03:40     INFO -  ###!!! [Parent][MessageChannel] Error: (msgtype=0xAA001E,name=PPluginInstance::Msg_AsyncSetWindow) Channel error: cannot send/recv
> 07:03:40     INFO -  ###!!! [Parent][OnMaybeDequeueOne] Error: Channel error: cannot send/recv
> 07:03:43     INFO -  1454425423606	addons.productaddons	INFO	sending request to: http://127.0.0.1:8888/dummy-gmp-manager.xml
> 07:03:43     INFO -  1454425423629	addons.productaddons	INFO	Completed downloading document
> 07:04:24     INFO -  TEST-INFO | started process screentopng
> 07:04:26     INFO -  TEST-INFO | screentopng: exit 0
> 07:04:26     INFO -  72 INFO checking window state
> 07:04:26     INFO -  73 INFO Entering test
> 07:04:26     INFO -  74 INFO Leaving test
> 07:04:26     INFO -  75 INFO Entering test
> 07:04:26     INFO -  76 INFO Wait tab event: load
> 07:04:26     INFO -  77 INFO Tab event received: load
> 07:04:26     INFO -  78 INFO Console message: 1454425423602	Toolkit.GMP	INFO	GMPInstallManager.simpleCheckAndInstall A version change occurred. Ignoring media.gmp-manager.lastCheck to check immediately for new or updated GMPs.
> 07:04:26     INFO -  79 INFO Console message: 1454425423602	Toolkit.GMP	INFO	GMPInstallManager._getURL Using override url: http://127.0.0.1:8888/dummy-gmp-manager.xml
> 07:04:26     INFO -  80 INFO Console message: 1454425423605	Toolkit.GMP	INFO	GMPInstallManager._getURL Using url (with replacement): http://127.0.0.1:8888/dummy-gmp-manager.xml
> 07:04:26     INFO -  81 INFO Console message: 1454425423606	addons.productaddons	INFO	sending request to: http://127.0.0.1:8888/dummy-gmp-manager.xml
> 07:04:26     INFO -  82 INFO Console message: 1454425423629	addons.productaddons	INFO	Completed downloading document
> 07:04:26     INFO -  83 INFO Console message: [JavaScript Error: "1454425423631	Toolkit.GMP	ERROR	GMPInstallManager.simpleCheckAndInstall Could not check for addons: Error: got node name: html, expected: updates (resource://gre/modules/addons/ProductAddonChecker.jsm:145:11) JS Stack trace: parseXML@ProductAddonChecker.jsm:145:11 < promise callback*ProductAddonChecker.getProductAddonList@ProductAddonChecker.jsm:299:12 < GMPInstallManager.prototype.checkForAddons@GMPInstallManager.jsm:107:5 < GMPInstallManager.prototype.simpleCheckAndInstall<@GMPInstallManager.jsm:204:29 < gBrowserInit._delayedStartup/<@browser.js:1316:7 < setTimeout handler*gBrowserInit._delayedStartup@browser.js:1312:5 < EventListener.handleEvent*gBrowserInit.onLoad@browser.js:1029:5 < onload@browser.xul:1:1" {file: "resource://gre/modules/Log.jsm" line: 751}]
> 07:04:26     INFO -  84 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js | Test timed out -




The "A version change occurred" it's referring to is expected, due to tests being run in a clean profile. Relevant to bug 1243690, but there's nothing there that particularly helps with this failure, unfortunately :\ Think we can remove that extra logging now (patch to do that in a sec).
Attachment #8712452 - Attachment is obsolete: true
Attachment #8715622 - Flags: review?(dtownsend)
Attachment #8715622 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/20a5e16ef24e
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla47 → ---
This one of the most-frequent mochitest-bc failures I'm seeing on OSX e10s Try runs. Do you have time to investigate this, Trevor?
Flags: needinfo?(smokey101stair)
Yes, I can take a look at this.  I don't have a Mac to test this on, but it looks like linux is affected as well, so I'll have to revive my linux VM dev environment.
Flags: needinfo?(smokey101stair)
Summary: Intermittent e10s browser_pluginCrashCommentAndURL.js | Test timed out → Intermittent e10s browser_pluginCrashCommentAndURL.js | The crash UI should be visible - false == true - | Test timed out
Sebastian: Where did you get the new summary for this bug? All of the error logs I have read have errors that look like this:

60 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js | Test timed out -

I have not seen any that say "The crash UI should be visible - false == true -"
Flags: needinfo?(aryx.bugmail)
Ah. I did find one that says this: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=23123617
I guess some tests seem to be failing in different ways.
Flags: needinfo?(aryx.bugmail)
Kirk, I hope you don't mind, but I finally got my Linux build working and have been poking at this.

Moving |submitButton.click()| after the |Assert.equal| calls seems to fix the "The crash UI should be visible - false == true - | Test timed out" errors for me locally.  This appears to be a timing issue where we are in the middle of a reflow and getting back an incorrect value after clicking the submit button.  Asserting that the crash UI is visible before attempting to submit the crash report is probably a better way of doing this anyway.

I'll push this to try once I get my try server account reactivated.
Attachment #8715622 - Attachment is obsolete: true
Attachment #8732450 - Flags: review?(dtownsend)
The issue with the test not receiving the |PluginCrashed| event is still a problem and I have been unable to reproduce it locally on my Linux VM.

It's a bit odd though, the screenshot shows that the crash UI is visible, which means that 1) the plugin did actually crash and 2) that the content process is actually hearing about it as there is an event listener in PluginContent.jsm[1] for the |PluginCrashed| event, which is what handles setting the appropriate state attribute to show the crash UI.  The only difference between the two event listeners is that the one in PluginContent.jsm listens during the capturing phase, while the listener in the test listens during the bubbling phase. I very much doubt that the event phase has anything to do with the underlying problem, though.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#42
Well, it looks like try proved me wrong, but I don't see a way to cancel a review request :(
on the attachment, click on details and you can adjust the review field
Comment on attachment 8732450 [details] [diff] [review]
bug1235056_patch.diff

Cancelling review. Thanks for the tip, Joel.
Attachment #8732450 - Flags: review?(dtownsend)
It's weird that you're seeing the log output before the assertion output: http://hg.mozilla.org/try/file/709c7ecd4df7568ff846ad8206e09352fe59d443/browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js#l94 I wonder if that's just a weirdness of the test harness.

To further increase logging, you could start logging not just in the test but in the bindings and submission code itself. For example:

http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/CrashSubmit.jsm#350 is where we fire "crash-report-status" in the content process.
http://mxr.mozilla.org/mozilla-central/source/toolkit/pluginproblem/content/pluginProblem.xml?force=1#74 is where the XBL constructor fires, but layout is not yet complete.

The "promiseUpdatePluginBindings" function is supposed to fix that, but I wonder whether that's not working properly. Is there only one plugin in this test?

http://mxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#362 is where the content process handles PluginBindingAttached and does the size checking and whatnot. That should end up in onPluginCrashed in the same file, which has this interesting nugget: "We haven't received information from the parent yet about this crash, so we should hold off showing the crash report UI." That looks suspicious and worth of additional logging.

The "plugin process has crashed" notification comes through "NPAPIPluginProcessCrashes", and I'm wondering if that function is broken. It seems that this could be inherently racy: if there are multiple plugin instances running for the same plugin, then if one of them crashes while the other is loading but not yet inserted into the contentWindowUtils.plugins array, we might never set its pluginCrashData map. Also, if there are multiple plugins on the page, one of which has already processed the plugin-crashed event but the other hasn't, the map will be out of sync at http://mxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#887

I wonder whether we could make this map permanent per toplevel page load to avoid this sort of inconsistency. I'm only guessing whether this is causing the problem here, though.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #36)
> It's weird that you're seeing the log output before the assertion output:
> http://hg.mozilla.org/try/file/709c7ecd4df7568ff846ad8206e09352fe59d443/
> browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js#l94 I
> wonder if that's just a weirdness of the test harness.
That is odd.

> To further increase logging, you could start logging not just in the test
> but in the bindings and submission code itself. For example:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/
> CrashSubmit.jsm#350 is where we fire "crash-report-status" in the content
> process.
> http://mxr.mozilla.org/mozilla-central/source/toolkit/pluginproblem/content/
> pluginProblem.xml?force=1#74 is where the XBL constructor fires, but layout
> is not yet complete.
> 
> The "promiseUpdatePluginBindings" function is supposed to fix that, but I
> wonder whether that's not working properly. Is there only one plugin in this
> test?
Yes, there is only one plugin in this test.

> http://mxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.
> jsm#362 is where the content process handles PluginBindingAttached and does
> the size checking and whatnot. That should end up in onPluginCrashed in the
> same file, which has this interesting nugget: "We haven't received
> information from the parent yet about this crash, so we should hold off
> showing the crash report UI." That looks suspicious and worth of additional
> logging.
Screenshots from the test harness show that the crash report UI is showing as expected, so I would think that this is working.

> The "plugin process has crashed" notification comes through
> "NPAPIPluginProcessCrashes", and I'm wondering if that function is broken.
> It seems that this could be inherently racy: if there are multiple plugin
> instances running for the same plugin, then if one of them crashes while the
> other is loading but not yet inserted into the contentWindowUtils.plugins
> array, we might never set its pluginCrashData map. Also, if there are
> multiple plugins on the page, one of which has already processed the
> plugin-crashed event but the other hasn't, the map will be out of sync at
> http://mxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.
> jsm#887
From my understanding, the "NPAPIPluginProcessCrashed" event only occurs when the actual plugin process crashes (i.e. plugin-container.exe) and not just when the plugin instance has crashed.  Is my understanding incorrect?

I did notice, that in the handleEvent method, https://dxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#382, we explicitly try to handle a "PluginCrashed" binding, however, _getBindingType, https://dxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#295, will always return null if the plugin has crashed.  I have filed bug 1258772 about this.

> I wonder whether we could make this map permanent per toplevel page load to
> avoid this sort of inconsistency. I'm only guessing whether this is causing
> the problem here, though.
I did suspect the Map of being a possible issue here, but if that were the case, then I would expect this to be failing on Windows as well.
I have discovered something that I had missed. Every time that I have seen an error (with all my extra logging enabled), I have observed that pluginCrashed resolved before promiseUpdatePluginBindings(gTestBrowser). Looking through a sample of logs, I have seen this happen only rarely when the test passes.
(In reply to Kirk Steuber [:ksteuber] from comment #40)
> I have discovered something that I had missed. Every time that I have seen
> an error (with all my extra logging enabled), I have observed that
> pluginCrashed resolved before promiseUpdatePluginBindings(gTestBrowser).
> Looking through a sample of logs, I have seen this happen only rarely when
> the test passes.

That is likely an inherit flaw of how the test is setup.  The actual call to crash the plugin is done in an inline script in the head of the HTML file that gets loaded.  This likely explains why you sometimes see promisePluginCrashed resolving before promiseUpdatePluginBindings.  You could try feeding the test a different HTML file and then calling promiseCrashObject[1] from the test script after promiseUpdatePluginBindings resolves.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/head.js#201
Kirk, I tried your changes from comment 42 locally and the test perma failed on Windows with e10s (although it worked fine on Windows non-e10s).  The e10s errors all seemed to point to promiseCrashObject resolving before the plugin had actually finished crashing.  I got around this by adding the following after the try catch block in promiseCrashObject:

yield ContentTaskUtils.waitForCondition(() => {
  return objLoadingContent.pluginFallbackType == objLoadingContent.PLUGIN_CRASHED;
}, 'The plugin should have crashed.');

I tried using |yield ContentTaskUtils.waitForEvent(content, "PluginCrashed", true);|, however, everything that I tried passing as the first argument didn't work.
Trevor: Maybe I am misunderstanding, but I do not think that it should be required that the plugin finish crashing at that point. The tests wait on `pluginCrashed` right after that.
Hi Kirk, the error that I was getting when running this in e10s was: TypeError: doc.getAnonymousElementByAttribute(...) is null.

I *think* what I was seeing in comment 43 is that the parent process is hearing about the "PluginCrashed" event before the child process.  "promisePluginCrashed" listens for the "PluginCrashed" event in the parent process, which then allows the test to continue before the listener in PluginContent.jsm in the child process has a chance to process the event.  Comments in https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_pluginCrashReportNonDeterminism.js#12 state that there is no guarantee about which process hears about the "PluginCrashed" event first. I could be wrong though.
In the last few try pushes, browser_pluginCrashReportNonDeterminism.js is perma failing because it also relies on the same HTML file, plugin_crashCommentAndURL.html, as this test.  When you removed the <body onload="crash();"> you made browser_pluginCrashReportNonDeterminism.js not work anymore.

There are no other consumers of the promiseCrashObject function, so it should be safe to use that without having to duplicate the logic in the test.
I finally got the bug to reproduce in rr, so I am going to spend some time going through the trace to see what I can find.
I believe that Mike Conley and I have discovered the source of the bug. The problem seems to be that the PluginCrashed event sometimes fires from the content process before the Chrome process has a chance process to finish handing the plugin crash. The Plugin Crash UI is not actually ready to use at this point. To make sure that the UI is actually ready, we should wait until setCrashedNPAPIPluginState()[1] has executed. I am running an attempt at this through try[2] and so far it seems to be passing. I will upload a patch once the tests finish and I am reasonably sure that this fixes the bug.

[1] http://searchfox.org/mozilla-central/source/browser/modules/PluginContent.jsm#911
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=45afb4bd7bfe
Assignee: nobody → ksteuber
Comment on attachment 8736786 [details]
MozReview Request: Bug 1235056 - browser_pluginCrashCommentAndURL.js test needs to wait for the crash reporter to be displayed, not just for the plugin to crash

https://reviewboard.mozilla.org/r/43553/#review40127

This Kirk! Glad this fixes things! I've got some suggestions here.

::: browser/modules/PluginContent.jsm:972
(Diff revision 1)
> +    // Notify others that the crash reporter UI is now ready
> +    this.global.content.dispatchEvent(
> +      new this.global.content.CustomEvent("PluginCrashReporterDisplayed", {bubbles: true}));

I think this is a fine name for this event.

We should actually probably only fire this if the crash reporter is being shown, so probably within the isShowing block that starts on 953.

We also might want to dispatch the event not on the content, but on the plugin itself... what I do want to ensure though is that web content cannot capture that event. Can you quickly test to see if that is the case?

Finally, I think this.global.content can be shortened to this.content, since I believe we set that alias in PluginContent.init.
Attachment #8736786 - Flags: review?(mconley)
Comment on attachment 8736947 [details]
MozReview Request: Bug 1235056 - PluginCrashReporterDisplayed event now only shown if the crash reporter can actually be shown and event is not accessible to web content.

Review commit: https://reviewboard.mozilla.org/r/43599/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43599/
Comment on attachment 8736947 [details]
MozReview Request: Bug 1235056 - PluginCrashReporterDisplayed event now only shown if the crash reporter can actually be shown and event is not accessible to web content.

Review commit: https://reviewboard.mozilla.org/r/43599/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43599/
Attachment #8736947 - Attachment is obsolete: true
Attachment #8736947 - Flags: review?(mconley)
Attachment #8736952 - Flags: review?(mconley)
Comment on attachment 8736952 [details]
MozReview Request: Bug 1235056 - PluginCrashReporterDisplayed event now only shown if the crash reporter can actually be shown and event is not accessible to web content.

https://reviewboard.mozilla.org/r/43605/#review40173

This looks good! Assuming a green try run (with retriggers to smoke out the orange), I think we can land this with fixes.

What I'd suggest is that you fold this into the previous commit once you address the issues I've opened. Let me know if you need guidance on how to do that.

::: browser/modules/PluginContent.jsm:960
(Diff revision 1)
>        // notification bar, then remove it because this plugin instance it big
>        // enough to serve as in-content notification.
>        this.hideNotificationBar("plugin-crashed");
>        doc.mozNoPluginCrashedNotification = true;
> +
> +      // Notify others that the crash reporter UI is now ready

Maybe mention that so far, this is just being used for tests.

::: browser/modules/PluginContent.jsm:961
(Diff revision 1)
> +      let winUtils = this.content.QueryInterface(Ci.nsIInterfaceRequestor)
> +                             .getInterface(Ci.nsIDOMWindowUtils);

Formatting nit - please align the periods, like so:

```
let winUtils = this.content.QueryInterface(Ci.nsIInterfaceRequestor)
                           .getInterface(Ci.nsIDOMWindowUtils);
```
Attachment #8736952 - Flags: review?(mconley) → review+
Comment on attachment 8736786 [details]
MozReview Request: Bug 1235056 - browser_pluginCrashCommentAndURL.js test needs to wait for the crash reporter to be displayed, not just for the plugin to crash

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43553/diff/1-2/
Attachment #8736952 - Attachment is obsolete: true
Attachment #8736786 - Flags: review?(mconley) → review+
Comment on attachment 8736786 [details]
MozReview Request: Bug 1235056 - browser_pluginCrashCommentAndURL.js test needs to wait for the crash reporter to be displayed, not just for the plugin to crash

https://reviewboard.mozilla.org/r/43553/#review40187

Assuming a green try build with retriggers to ensure the orange is gone, r=me! Great job, Kirk!
(In reply to Kirk Steuber [:ksteuber] from comment #64)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=970f0433d58d

That is one green test. :) LAND LAND LAND!
https://hg.mozilla.org/mozilla-central/rev/326505923c0f
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(ksteuber)
Comment on attachment 8736786 [details]
MozReview Request: Bug 1235056 - browser_pluginCrashCommentAndURL.js test needs to wait for the crash reporter to be displayed, not just for the plugin to crash

Approval Request Comment
[Feature/regressing bug #]:
Fix for intermittent orange: Bug 1235056

[User impact if declined]:
None

[Describe test coverage new/current, TreeHerder]:
Test is passing consistently with patch applied:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=970f0433d58d&selectedJob=18888621

[Risks and why]:
Low Risk. Only change to Firefox is dispatch a signal once the Plugin Crash Reporter UI is displayed.

[String/UUID change made/needed]:
None
Flags: needinfo?(ksteuber)
Attachment #8736786 - Flags: approval-mozilla-beta?
Attachment #8736786 - Flags: approval-mozilla-aurora?
Comment on attachment 8736786 [details]
MozReview Request: Bug 1235056 - browser_pluginCrashCommentAndURL.js test needs to wait for the crash reporter to be displayed, not just for the plugin to crash

Fixes an intermittent, Aurora47+
Attachment #8736786 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8736786 [details]
MozReview Request: Bug 1235056 - browser_pluginCrashCommentAndURL.js test needs to wait for the crash reporter to be displayed, not just for the plugin to crash

Fix for annoying intermittent failure on crash tests, should be ok to uplift to beta.
Attachment #8736786 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.