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)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: aryx, Assigned: bytesized)
References
(Blocks 1 open bug)
Details
(Keywords: intermittent-failure)
Attachments
(2 files, 4 obsolete files)
2.41 KB,
patch
|
Details | Diff | Splinter Review | |
58 bytes,
text/x-review-board-request
|
mconley
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details |
+++ 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
Updated•8 years ago
|
Blocks: e10s-tests
tracking-e10s:
--- → +
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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+
Comment 7•8 years ago
|
||
(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 :)
Comment 9•8 years ago
|
||
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.
Comment 10•8 years ago
|
||
(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.
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0ecd7d72f232
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•8 years ago
|
Status: RESOLVED → REOPENED
status-firefox47:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla47 → ---
Comment 12•8 years ago
|
||
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).
Comment 13•8 years ago
|
||
Attachment #8712452 -
Attachment is obsolete: true
Attachment #8715622 -
Flags: review?(dtownsend)
Updated•8 years ago
|
Attachment #8715622 -
Flags: review?(dtownsend) → review+
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/20a5e16ef24e
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•8 years ago
|
Status: RESOLVED → REOPENED
status-firefox47:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla47 → ---
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Updated•8 years ago
|
status-firefox45:
--- → wontfix
status-firefox47:
--- → affected
status-firefox-esr45:
--- → unaffected
Comment 19•8 years ago
|
||
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?
Blocks: e10s-tests-osx
Flags: needinfo?(smokey101stair)
Comment 20•8 years ago
|
||
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)
Comment hidden (Intermittent Failures Robot) |
Reporter | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=785ba67dfedf
Assignee | ||
Comment 23•8 years ago
|
||
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)
Assignee | ||
Comment 24•8 years ago
|
||
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)
Reporter | ||
Comment 25•8 years ago
|
||
The test output changed after bug 1241930 landed. Relevant change: https://hg.mozilla.org/mozilla-central/rev/4247cb5030a6#l43.57 You can find these failures with brasstacks, e.g. https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1235056&startday=2016-03-01&endday=2016-03-11&tree=mozilla-inbound Test output on OS X 10.6 before change: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=22917515 after change: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=23554411
Assignee | ||
Comment 26•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ddfc4417b8d
Comment hidden (Intermittent Failures Robot) |
Comment 28•8 years ago
|
||
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)
Comment 29•8 years ago
|
||
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
Comment 31•8 years ago
|
||
Well, it looks like try proved me wrong, but I don't see a way to cancel a review request :(
Comment 32•8 years ago
|
||
on the attachment, click on details and you can adjust the review field
Comment 33•8 years ago
|
||
Comment on attachment 8732450 [details] [diff] [review] bug1235056_patch.diff Cancelling review. Thanks for the tip, Joel.
Attachment #8732450 -
Flags: review?(dtownsend)
Comment hidden (Intermittent Failures Robot) |
Comment 36•8 years ago
|
||
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.
Comment 38•8 years ago
|
||
(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.
Assignee | ||
Comment 39•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb6a75abe502
Assignee | ||
Comment 40•8 years ago
|
||
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.
Comment 41•8 years ago
|
||
(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
Assignee | ||
Comment 42•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d50efdc66ada
Comment 43•8 years ago
|
||
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.
Assignee | ||
Comment 44•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0536ee49a8be
Assignee | ||
Comment 45•8 years ago
|
||
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.
Assignee | ||
Comment 46•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c1d9b537322
Comment 47•8 years ago
|
||
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.
Comment 48•8 years ago
|
||
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.
Assignee | ||
Comment 49•8 years ago
|
||
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.
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 51•8 years ago
|
||
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
Assignee | ||
Comment 52•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03b2f219f11e
Assignee | ||
Comment 54•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43553/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43553/
Attachment #8736786 -
Flags: review?(mconley)
Comment 55•8 years ago
|
||
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)
Assignee | ||
Comment 56•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43599/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43599/
Attachment #8736947 -
Flags: review?(mconley)
Attachment #8736786 -
Flags: review?(mconley)
Assignee | ||
Comment 57•8 years ago
|
||
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/
Assignee | ||
Comment 58•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Attachment #8736947 -
Attachment is obsolete: true
Attachment #8736947 -
Flags: review?(mconley)
Assignee | ||
Comment 59•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43605/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43605/
Assignee | ||
Updated•8 years ago
|
Attachment #8736952 -
Flags: review?(mconley)
Assignee | ||
Comment 60•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec775180dd8d
Comment 61•8 years ago
|
||
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+
Assignee | ||
Comment 62•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Attachment #8736952 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8736786 -
Flags: review?(mconley) → review+
Comment 63•8 years ago
|
||
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!
Assignee | ||
Comment 64•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=970f0433d58d
Comment 65•8 years ago
|
||
(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!
Comment 67•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/326505923c0f
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 68•8 years ago
|
||
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(ksteuber)
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 70•8 years ago
|
||
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 72•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc5fc975a47a
Comment 73•8 years ago
|
||
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+
Comment 74•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/d117771dc5cf
You need to log in
before you can comment on or make changes to this bug.
Description
•