Closed Bug 870290 Opened 7 years ago Closed 6 years ago

[SeaMonkey] (perma-orange) TEST-UNEXPECTED-FAIL test_hang_submit.xul | Test timed out. (Broken plugin crash reporter submit link)


(SeaMonkey :: General, defect, major)

Not set


(seamonkey2.21 fixed)

Tracking Status
seamonkey2.21 --- fixed


(Reporter: mcsmurf, Assigned: mcsmurf)


(Keywords: intermittent-failure)


(1 file, 2 obsolete files)

Currently these tests time out:
3852 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_crash_submit.xul | Test timed out.
3857 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_hang_submit.xul | Test timed out.

This is probably due to this code trying from notification.xml trying to modify some non-existing link:
2111               let pleaseLink = doc.getAnonymousElementByAttribute(
2112                                     plugin, "class", "pleaseSubmitLink");
2113               this.addLinkClickCallback(pleaseLink, this.submitReport,
2114                                         pluginDumpID, browserDumpID);

There is no element with the pleaseSubmitLink CSS class.
Basically need to port fixes from Bug 648675.
Attached patch Patch (obsolete) — Splinter Review
This patch ports the changes from Firefox. The patch basically adjust the SeaMonkey UI to the toolkit/ changes. It added a comment box and a checkbox enabling the user to send the page URL to Mozilla for sending plugin crash info via crashreporter. This patch adjusts the SeaMonkey JS and CSS code to make sure everything gets displayed fine: It checks if the plugin area is big enough to display the plugin crash UI, if not, it will display a notification bar. The patch also adjusts the Modern theme CSS rules to the Firefox version of pluginProblem.css. And the patch copies the Firefox test to make sure the new UI works as expected (needed to adjust a few lines to make the test work in SeaMonkey).
Original FF changeset:
Assignee: nobody → bugzilla
Attachment #747978 - Flags: review?(philip.chee)
I did get one error running the tests:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_pluginCrashCom
mentAndURL.js | leaked until shutdown [nsGlobalWindow #20]
Depends on Bug 798278 in some way as Bug 798278 fixes a JavaScript error that occurs before displaying the plugin crash UI (and currently stops JS execution)
Depends on: 798278
Comment on attachment 747978 [details] [diff] [review]

Canceling review for now.
Attachment #747978 - Flags: review?(philip.chee)
> +      <method name="getPluginUI">
File a followup bug to replace (where sensible) calls to Plugin.ownerDocument.getAnonymousElementByAttribute() with this method.

> -            this.CrashSubmit.submit(pluginDumpID);
> +            let keyVals = {};

var keyVals = {}

> -              this.addLinkClickCallback(pleaseLink, this.submitReport,
> -                                        pluginDumpID, browserDumpID);
> +              this.getPluginUI(plugin, "submitButton").addEventListener("click",

Hmm I still prefer addLinkClickCallback() assuming that it works.

> +                  pref.setBoolPref("", optInCB.checked);
> +                }.bind(this));

> +                let optInCB = this.getPluginUI(plugin, "submitURLOptIn");
Further up there are two calls to getAnonymousElementByAttribute() which can be replaced by this.getPluginUI().

> +                let optInCB = this.getPluginUI(plugin, "submitURLOptIn");
> +                let pref = Services.prefs.getBranch("dom.ipc.plugins.reportCrashURL");
> +                optInCB.checked = pref.getBoolPref("");

Incorrect indentation in the previous three lines.

Please don't introduce a dependency on Services.jsm use this._prefs instead.
Also using getBranch just makes the code less readable.
Do something like this (untested code):

  var submitButton = this.getPluginUI(plugin, "submitButton");
  var optInCB = this.getPluginUI(plugin, "submitURLOptIn");
  optInCB.checked = this._prefs.getBoolPref("dom.ipc.plugins.reportCrashURL");
  var submitReport = function(pluginDumpID, browserDumpID, plugin, optInCB) {
                       this.submitReport(pluginDumpID, browserDumpID, plugin);
                       this._prefs.setBoolPref("dom.ipc.plugins.reportCrashURL", optInCB.checked);
  this.addLinkClickCallback(submitButton, submitReport, pluginDumpID, browserDumpID, plugin, optInCB);

> +            // First try hiding the comment box and related report submission UI.
> +            var submitDiv =
> +              doc.getAnonymousElementByAttribute(plugin, "class",
> +                                                 "msg msgPleaseSubmit");
> +   = "none";

Remove this hunk as the final Firefox patch doesn't include this.

> +  let tab = gBrowser.loadOneTab("about:blank", { inBackground: false });
> +  let browser = gBrowser.getBrowserForTab(tab);
let browser = tab.linkedBrowser;

> +  let env = Cc[";1"].
> +            getService(Components.interfaces.nsIEnvironment);
Either use short cuts (Cc, Ci, Cr) consistently or spell these out in full (Neil prefers no shortcuts).

> +    let plugin = gBrowser.contentDocument.getElementById("plugin");

> +    let elt = plugin.ownerDocument.getAnonymousElementByAttribute.bind(plugin.ownerDocument, plugin, "class");
Please use getBrowser() instead of gBrowser.
Isn't gBrowser.contentDocument == plugin.ownerDocument ?
I think the following should work:

let elt = getBrowser().getNotificationBox().getPluginUI;
elt(plugin, "submitComment").value = currentRun.comment;
Attached patch Patch (obsolete) — Splinter Review
Ratty: I've fixed almost all review comment except the ones where I think there's a better solution :)
- Regarding addLinkClickCallback: My code does more than just adding a callback for sending a crash report, it also sets the dom.ipc.plugins.reportCrashURL pref depending on a checkbox in the UI. I'm against modifying the submitReport method so that it will also change the pref. submitReport should just collect the required information and send the crash report. I could also add a new function but that would be even more overhead.
- In the test I used Components.results in one line as Cr does not work yet in SeaMonkey tests, I'll file a follow-up bug on that
- I'm against using "let elt = getBrowser().getNotificationBox().getPluginUI;" in the test, that looks like a bad hack to me. If I use getNotificationBox(), I expect the code to do something regarding notifications. If I just use it to call some random function inside notification.xml...I certainly don't like that. Let's keep the code clean and just use two function calls to get to the plugin object.
- All other review comments have been fixed
Attachment #747978 - Attachment is obsolete: true
Attachment #760189 - Flags: review?(philip.chee)
Comment on attachment 760189 [details] [diff] [review]

[Triage Comment]

> +++ b/suite/common/bindings/notification.xml

> -          var crashText = doc.getAnonymousElementByAttribute(plugin, "class", "msg msgCrashed");
> +          var crashText = doc.getAnonymousElementByAttribute(plugin, "class", "msgCrashedText");
Any reason you aren't using getPluginUI() here?

> +++ b/suite/browser/test/browser/browser_pluginCrashCommentAndURL.js
> +            getService(Ci.nsIEnvironment);
Firefox uses: getService(Components.interfaces.nsIEnvironment);
I think we should be consistent. Either all in full, or all using Cu/Cc/Ci/Cr.

> +++ b/suite/themes/modern/mozapps/plugins/pluginProblem.css

Please file a followup bug to port:
Bug 831921 - Make the plugin UI less broken-looking for all plugins except for crashed plugins, including visual tweaks by shorlander.
Bug 842692 - Plugin click-to-play play button has default cursor.

Sorry for the extreme delay.

a=me for comm-aurora
Attachment #760189 - Flags: review?(philip.chee)
Attachment #760189 - Flags: review+
Attachment #760189 - Flags: approval-comm-aurora+
Fixed the getPluginUI thing.
I want to use Ci/Cc/... everywhere, but we can't use Cr yet, need to file a bug on that/fix that (that is why I used Components.results in the test file).
Attachment #760189 - Attachment is obsolete: true
Comment on attachment 769642 [details] [diff] [review]
Patch for checkin

Pushed to comm-central:
Target Milestone: --- → seamonkey2.22
Wrong dependency.
No longer depends on: 798278
Fixed the line endings of the test file (from DOS to Unix):
Fixed the line endings of the test file (from DOS to Unix):
All fixed here.
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.