SM trunk with OOPP on win32 a) load youtube and start a video b) kill plugin-container.exe with the taskmanager c) the plugin space on the page becomes white Do the same with FF and the Plugin space is replaced with a crash message. Firefox implemented this with bug 538910

Blocking b2 due to l10n impact, but I do _want_ this for b1.

Can we please get some traction here? This is marked blocking beta 2...

Should this depend on bug 595810?

Will try to drive this in for b3, re-nomming for blocking though, as I'm not sure we still want to block on it. Note this has l10n-impact, but if desired I can land strings before UI and let this stretch to final.

Yes, land the strings in time for beta 3 if you cannot get it all in for beta 3.

Note that this is somewhat related to bug 521159.

Ok Neil, This is a mess to TRY and do in the binding with the rest of our plugin* stuff. can I please please do this in navigator.js similar to: http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#6600 Thanks.

I'm currently looking into adapting the Firefox code into SeaMonkey. This is complicated due to Neil totally rewriting our notifications code so nobody but Neil understands how the new code works. Also the Firefox code is fairly convoluted. Because gPluginHandler does so many things, Firefox has spun off a bunch of helper methods to improve code reuse. However this makes the crashedPlugin logic harder to unwind. Unless Callek wants to do the port, I'm going to work on it (actually I'm about 1/3 of the way through).

Comment on attachment 520491 [details] [diff] [review] l10n only patch [checked in Comment 32] What about carbonfailureplugins?

(In reply to comment #8) > This is a mess to TRY and do in the binding with the rest of our plugin* stuff. Using XBL isn't much more different than using gPluginHandler. There are of course some obvious differences: 1. Instead of adding an event listener to the browser, you add an XBL handler. 2. The methods on gPluginHandler become XBL methods. 3. You use this.activeBrowser instead of calling getBrowserForDocument. 4. You use this instead of calling getNotificationBox. 5. We store missingPlugins on the XBL too. What doesn't help is that Firefox rewrote their code since I ported it.

(In reply to comment #9) > However this makes the > crashedPlugin logic harder to unwind. Well, I'd not have a problem if you'd get outdated plugins (bug 521159) into the same bug - and I'm sure Adrian also wouldn't mind. ;-)

Anybody else wants to do a drive by feedback review, please feel free.

> Anybody else wants to do a drive by feedback review, please feel free. Hack and Slash job. Needs lots of cleaning up but it works AFAICT Modern fixes not included yet.

(In reply to comment #0) > SM trunk with OOPP on win32 > > a) load youtube and start a video > b) kill plugin-container.exe with the taskmanager > c) the plugin space on the page becomes white > > Do the same with FF and the Plugin space is replaced with a crash message. > Firefox implemented this with bug 538910 So, for me the only difference with and without the patch is that with the patch I get a message ("The Adobe Flash plugin has crashed."), but even without I get a grey striped background and a "sad plugin" icon. I'm on Win7 with full HW accel, if that matters. So, what is this bug supposed to provide exactly and which part of that does the WIP patch provide? [Please read as a try to clarify STR and feedback quality, not by any means an accusation!]

I tested Minefield and my patch side by side. Same as you. Load flash, kill plugin-container, compare UI. I fiddled with the patch until I got exactly the same result. Make sure you are testing on Classic. I haven't started on the Modern CSS. What you should see is: 1. Fail plugin icon. 2. The Adobe Flash plugin has crashed 3. A link that says "Reload page". 3a. Clicking on the link goes to a non existent page at seamonkey-projects org at the moment. 4. Bottom left corner a (?) icon - code calls it a helpIcon. 4a. Clicking on it brings you to the same page as 3a. Additionally if the plugin had a real crash instead of artificially killing it off with the task manager there will be a minidump. The code should detect the minidump and an additional link to "Submit a crash report". Additionally^2 if one flash object on that page is too small to show the fail whale the notification bar will pop up: [The Adobe Flash plugin has crashed] [Learn More...] <...spacer...> [Help] [Submit a crash report] [X] Tested on http://hardocp.com/ Make sure you don't have Flashblock or AdblockPlus or NoScript installed.

Sorry > 3a. Clicking on the link goes to a non existent page at seamonkey-projects org at the moment. This Should be: 3a. Clicking on the link reloads the current page. 4a. Clicking on the link goes to a non existent page at seamonkey-projects org at the moment.

(In reply to comment #10) > (From update of attachment 520491 [details] [diff] [review]) > What about carbonfailureplugins? Ah, that's attachment 520951 [details] [diff] [review].

[After fixing WIP patch bitrot locally] I think I've found the issue: I'm building with --disable-crashreporter. That disables the code which sets the status attribute on submitStatus, which means that none of the rules in pluginProblemContent.css that set display:block apply so the default rule for .msg wins (which is display:none). AFAICS it's the same with FF, though they're using pre-processing for the differentiation. Still I think that's bad: Why should the reload text or help icon be disabled if the crash reporter is disabled? I think we must fix this (either in code or CSS), and then FF (or at least tell them). BTW 1: I've found that sometimes this._stringBundle is not set so it fails on the var messageString line. Test case: www.heise.de (seen with Chromebug 1.7 which I begin to heart, though it crashes for me with YouTube) BTW 2: I saw some JS errors when visiting http://www.adobe.com/shockwave/welcome/ with Flash installed but not Shockwave and then clicking the icon above "Click here to download plugin.". Not sure whether that can/should be fixed in this bug or not, though.

> I think I've found the issue: I'm building with --disable-crashreporter. That > disables the code which sets the status attribute on submitStatus, which means > that none of the rules in pluginProblemContent.css that set display:block apply > so the default rule for .msg wins (which is display:none). > > AFAICS it's the same with FF, though they're using pre-processing for the > differentiation. Still I think that's bad: Why should the reload text or help > icon be disabled if the crash reporter is disabled? I think we must fix this > (either in code or CSS), and then FF (or at least tell them). OK. I've moved the logic around so that the Reload link is always visible.

Comment on attachment 521396 [details] [diff] [review] Patch v1.0 Reload link always visible. (In reply to comment #20) > OK. I've moved the logic around so that the Reload link is always visible. I see the text about no available reports, the reload link and the help icon now, and the latter two work as expected. f=me For your consideration / code review: If (!this.CrashSubmit && pluginDumpID), the status variable is never set, and that becomes the value of the status attribute. Don't know whether the above combination is possible at all or whether setting the attribute to that value is a problem.

Comment on attachment 521396 [details] [diff] [review] Patch v1.0 Reload link always visible. >+ let callbackArgs = Array.prototype.slice.call(arguments).slice(2); Array.slice(arguments, 2) >+ function(evt) { I think we can spare the room to call this event ;-) >+ if (!evt.isTrusted) >+ return; >+ if (evt.keyCode == evt.DOM_VK_RETURN) { Nit: mix of styles; should early return here too. >+ // The crash reporter wants a DOM element it can append an IFRAME to, >+ // which it uses to submit a form. Let's just give it gBrowser. >+ this.CrashSubmit.submit(pluginDumpID, getBrowser(), null, null); >+ if (browserDumpID) >+ this.CrashSubmit.submit(browserDumpID, getBrowser(), null, null); Um, no, let's not give it gBrowser. This sucks, because CrashSubmit doesn't hide the iframe properly, but failing that, use this.activeBrowser please. >+ <!-- Callback for user clicking a "reload page" link --> >+ <method name="reloadPage"> >+ <parameter name="browser"/> >+ <body> >+ <![CDATA[ >+ browser.reload(); Don't need to pass the browser because it's always this.activeBrowser >+ var tmp = {}; >+ Components.utils.import("resource://gre/modules/CrashSubmit.jsm", tmp); >+ this.CrashSubmit = tmp.CrashSubmit; Can import directly into this, since CrashSubmit only exports one symbol. >+ if (this.CrashSubmit) { >+ // Determine which message to show regarding crash reports. Wrong indentation. >+ observe : function(subject, topic, data) { Nit: no space before : >+ // Clever solution? Use a closue with an event listener on the document. Typo: closure >+ // When the doc goes away, so do the listener references and the closure. >+ doc.addEventListener("mozCleverClosureHack", observer, false); Since this is here to update the statusDiv, I think we should add the event listener to it instead of the document. (Move the variable of course.) >+ function showNotificationBar(pluginDumpID, browserDumpID, nBox) { Make this a <method>. >+ callback: function() { browser.reload(); }, function() { this.activeBrowser.reload(); }.bind(this) >+ if (nBox.CrashSubmit) { >+ let submitButton = { >+ label: submitLabel, >+ accessKey: submitKey, >+ popup: null, >+ callback: function() { this.submitReport(pluginDumpID, browserDumpID); }.bind(nBox), Nit: indentation and tailing comma >+ if (pluginDumpID) >+ buttons.push(submitButton); Why not if (this.CrashSubmit && pluginDumpID) ? [Possibly could inline submitButton] >+ // Remove the notfication when the page is reloaded. >+ doc.defaultView.top.addEventListener("unload", function() { >+ this.removeNotification(notification); >+ }.bind(nBox), false); This should happen automatically, as it's a transient notification by default.

Comment on attachment 521396 [details] [diff] [review] Patch v1.0 Reload link always visible. >+ <parameter name="callbackName"/> I'm tempted to suggest you should pass the callback method directly. (Particularly convenient in the reload case, as you could then pass the same bound method that you need for the notification button callback.)

Comment on attachment 521396 [details] [diff] [review] Patch v1.0 Reload link always visible. >+ // Is the <object>'s size too small to hold what we want to show? >+ if (this.isTooSmall(plugin, overlay)) { >+ // Hide the overlay's contents. Use visibility style, so that it >+ // doesn't collapse down to 0x0. >+ overlay.style.visibility = "hidden"; >+ // If another plugin on the page was large enough to show our UI, we >+ // don't want to show a notification bar. >+ if (!doc.mozNoPluginCrashedNotification) >+ showNotificationBar(pluginDumpID, browserDumpID, this); >+ } else { >+ // If a previous plugin on the page was too small and resulted in >+ // adding a notification bar, then remove it because this plugin >+ // instance it big enough to serve as in-content notification. >+ var notification = this.getNotificationWithValue("plugin-crashed"); >+ if (notification) >+ this.removeNotification(notification, true); >+ doc.mozNoPluginCrashedNotification = true; >+ } For some reason I got both the notification bar and the in-content notification when trying to kill Flash on YouTube... mozNoPluginCrashedNotification should be a field that you reset on page change (just after clearing missing plugins, perhaps).

Comment on attachment 521396 [details] [diff] [review] Patch v1.0 Reload link always visible. >+ var newName = aName.replace(/\bplug-?in\b/i, "").replace(/[\s\d\.\-\_\(\)]+$/, ""); >+ return newName; ... >+ var overflows = (aOverlay.scrollWidth > pluginRect.width) || >+ (aOverlay.scrollHeight - 5 > pluginRect.height); >+ return overflows; Not worth separate variables.

(In reply to comment #24) > For some reason I got both the notification bar and the in-content notification > when trying to kill Flash on YouTube... There was an advert in a frame which was big enough for in-content notification, and another flash object that wasn't, but because it wasn't in the ad frame it triggered the notification bar, even though the ad crash was visible. > mozNoPluginCrashedNotification should be a field that you reset on page change This would fix the bug. It would also stop content pages from disabling the notification ;-) (You would want to use a better name though!)

(In reply to comment #21) > Jens Hatlak (:InvisibleSmiley) 2011-03-24 15:46:10 PDT > For your consideration / code review: If (!this.CrashSubmit && pluginDumpID), > the status variable is never set, and that becomes the value of the status > attribute. Don't know whether the above combination is possible at all or > whether setting the attribute to that value is a problem. Hmm. I assume that if the crash reporter isn't compiled in then no minidumps are created and hence no pluginDumpID's

> neil@parkwaycc.co.uk 2011-03-25 06:32:49 PDT > >>+ let callbackArgs = Array.prototype.slice.call(arguments).slice(2); > Array.slice(arguments, 2) Fixed. >>+ function(evt) { > I think we can spare the room to call this event ;-) Fixed. >>+ if (!evt.isTrusted) >>+ return; >>+ if (evt.keyCode == evt.DOM_VK_RETURN) { > Nit: mix of styles; should early return here too. Fixed. >>+ // The crash reporter wants a DOM element it can append an IFRAME to, >>+ // which it uses to submit a form. Let's just give it gBrowser. >>+ this.CrashSubmit.submit(pluginDumpID, getBrowser(), null, null); >>+ if (browserDumpID) >>+ this.CrashSubmit.submit(browserDumpID, getBrowser(), null, null); > Um, no, let's not give it gBrowser. This sucks, because CrashSubmit doesn't > hide the iframe properly, but failing that, use this.activeBrowser please. Fixed. >>+ <!-- Callback for user clicking a "reload page" link --> >>+ <method name="reloadPage"> >>+ <parameter name="browser"/> >>+ <body> >>+ <![CDATA[ >>+ browser.reload(); > Don't need to pass the browser because it's always this.activeBrowser Fixed. >>+ var tmp = {}; >>+ Components.utils.import("resource://gre/modules/CrashSubmit.jsm", tmp); >>+ this.CrashSubmit = tmp.CrashSubmit; > Can import directly into this, since CrashSubmit only exports one symbol. Fixed. >>+ if (this.CrashSubmit) { >>+ // Determine which message to show regarding crash reports. > Wrong indentation. Fixed. >>+ observe : function(subject, topic, data) { > Nit: no space before : Fixed. >>+ // Clever solution? Use a closue with an event listener on the document. > Typo: closure Fixed. >>+ // When the doc goes away, so do the listener references and the closure. >>+ doc.addEventListener("mozCleverClosureHack", observer, false); > Since this is here to update the statusDiv, I think we should add the event > listener to it instead of the document. (Move the variable of course.) > >>+ function showNotificationBar(pluginDumpID, browserDumpID, nBox) { > Make this a <method>. Fixed. >>+ callback: function() { browser.reload(); }, > function() { this.activeBrowser.reload(); }.bind(this) Fixed. >>+ if (nBox.CrashSubmit) { >>+ let submitButton = { >>+ label: submitLabel, >>+ accessKey: submitKey, >>+ popup: null, >>+ callback: function() { this.submitReport(pluginDumpID, browserDumpID); }.bind(nBox), > Nit: indentation and tailing comma Fixed. >>+ if (pluginDumpID) >>+ buttons.push(submitButton); > Why not if (this.CrashSubmit && pluginDumpID) ? Fixed. > [Possibly could inline submitButton] I decided that it was clearer if I didn't inline submitButton. >>+ // Remove the notfication when the page is reloaded. >>+ doc.defaultView.top.addEventListener("unload", function() { >>+ this.removeNotification(notification); >>+ }.bind(nBox), false); > This should happen automatically, as it's a transient notification by default. Removed. >>+ <parameter name="callbackName"/> > I'm tempted to suggest you should pass the callback method directly. > (Particularly convenient in the reload case, as you could then pass the same > bound method that you need for the notification button callback.) Fixed. Now passing the callback method directly. > mozNoPluginCrashedNotification should be a field that you reset on page change > (just after clearing missing plugins, perhaps). Fixed. >>+ var newName = aName.replace(/\bplug-?in\b/i, "").replace(/[\s\d\.\-\_\(\)]+$/, ""); >>+ return newName; > ... >>+ var overflows = (aOverlay.scrollWidth > pluginRect.width) || >>+ (aOverlay.scrollHeight - 5 > pluginRect.height); >>+ return overflows; > Not worth separate variables. Fixed. >> For some reason I got both the notification bar and the in-content notification >> when trying to kill Flash on YouTube... > There was an advert in a frame which was big enough for in-content > notification, and another flash object that wasn't, but because it wasn't in > the ad frame it triggered the notification bar, even though the ad crash was > visible. Should be fixed but I'm not sure how to test this. Got a suitable test page? >> mozNoPluginCrashedNotification should be a field that you reset on page change > This would fix the bug. It would also stop content pages from disabling the > notification ;-) (You would want to use a better name though!) Fixed. Now using "crashNotified"

Comment on attachment 522373 [details] [diff] [review] Patch v1.1 address review comments. >+ callback: function() { this.reloadPage(); }.bind(this) this.reloadPage.bind(this) >+ callback: function() { this.submitReport(pluginDumpID, browserDumpID); }.bind(this) this.submitReport.bind(this, pluginDumpID, browserDumpID) Neat huh? >+ link.href = this.crashReportHelpURL; This only works by accident (href is an XBL property, but you get to overwrite it because you haven't added the node to the document yet). And yes, I am one of the people who copied it from Firefox's original geolocation infobar... >- var formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"] >- .getService(Components.interfaces.nsIURLFormatter); >- link.href = formatter.formatURLPref("browser.geolocation.warning.infoURL"); >+ link.href = this._urlFormatter.formatURLPref("browser.geolocation.warning.infoURL"); > > document.getAnonymousElementByAttribute(box, "anonid", "messageText").appendChild(link); > }, 0, this._stringBundle); This doesn't work because it's in a setTimeout. You could try passing this._urlFormatter in as a second parameter though. >+ var overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox"); [Bah, why didn't Firefox give these elements proper anonids?] >+ status = "please"; >+ // XXX can we make the link target actually be blank? // XXX indentation still wrong (and in other places too) >+ this.addLinkClickCallback(pleaseLink, this.submitReport, >+ pluginDumpID, browserDumpID); Hmm... if we use this.submitReport.bind(this, pluginDumpID, browserDumpID) does that mean we can simplify addLinkClickCallback?

(In reply to comment #28) >>> For some reason I got both the notification bar and the in-content notification >>> when trying to kill Flash on YouTube... >> There was an advert in a frame which was big enough for in-content >> notification, and another flash object that wasn't, but because it wasn't in >> the ad frame it triggered the notification bar, even though the ad crash was >> visible. > Should be fixed but I'm not sure how to test this. Got a suitable test page? Sadly not; YouTube's home page doesn't have any Flash today...

Comment on attachment 520491 [details] [diff] [review] l10n only patch [checked in Comment 32] Requesting approval for the strings only patch in order to get these in before L10n freeze.

Comment on attachment 520491 [details] [diff] [review] l10n only patch [checked in Comment 32] Pushed to comm-central http://hg.mozilla.org/comm-central/rev/527695052f8d

>>+ callback: function() { this.reloadPage(); }.bind(this) > this.reloadPage.bind(this) Fixed. >>+ callback: function() { this.submitReport(pluginDumpID, browserDumpID); }.bind(this) > this.submitReport.bind(this, pluginDumpID, browserDumpID) > Neat huh? Fixed. >>+ link.href = this.crashReportHelpURL; > This only works by accident (href is an XBL property, but you get to overwrite > it because you haven't added the node to the document yet). And yes, I am one > of the people who copied it from Firefox's original geolocation infobar... Fixed. >>- var formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"] >>- .getService(Components.interfaces.nsIURLFormatter); >>- link.href = formatter.formatURLPref("browser.geolocation.warning.infoURL"); >>+ link.href = this._urlFormatter.formatURLPref("browser.geolocation.warning.infoURL"); >> >> document.getAnonymousElementByAttribute(box, "anonid", "messageText").appendChild(link); >> }, 0, this._stringBundle); > This doesn't work because it's in a setTimeout. You could try passing > this._urlFormatter in as a second parameter though. Passing this as the second parameter seems to work. Fixed. > // XXX indentation still wrong (and in other places too) I think I managed to get all of it. Fixed. >>+ this.addLinkClickCallback(pleaseLink, this.submitReport, >>+ pluginDumpID, browserDumpID); > Hmm... if we use this.submitReport.bind(this, pluginDumpID, browserDumpID) does > that mean we can simplify addLinkClickCallback? If you don't mind I'll leave this to a followup bug. > (In reply to comment #28) >>>> For some reason I got both the notification bar and the in-content notification >>>> when trying to kill Flash on YouTube... >>> There was an advert in a frame which was big enough for in-content >>> notification, and another flash object that wasn't, but because it wasn't in >>> the ad frame it triggered the notification bar, even though the ad crash was >>> visible. >> Should be fixed but I'm not sure how to test this. Got a suitable test page? > Sadly not; YouTube's home page doesn't have any Flash today... > > <NeilAway> Ratty: also, this is my infobar test url: data:text/html,<embed type="application/x-test"><iframe src="data:text/html,<embed height='100' type='application/x-test'>"> OK. Tested. Also Tested on http://espn.go.com/ where there is a small flash object to the left of the section titled "CRICKET WORLD CUP"

Comment on attachment 523214 [details] [diff] [review] Patch v1.2 Fix more nits [Checked in: Comment 35] >+ // Add the "learn more" link. >+ // XXXRatty We have a 404 at http://www.seamonkey-project.org/doc/plugin-crashed >+ // Or should we link to our in app help? Good question. I only just noticed that the help icon does something different to the Learn More link...

Comment on attachment 523214 [details] [diff] [review] Patch v1.2 Fix more nits [Checked in: Comment 35] Pushed to comm-central http://hg.mozilla.org/comm-central/rev/ec1cb7183b82 I noticed that ui-review wasn't asked for any of the other recent notification patches. > Good question. I only just noticed that the help icon does something different > to the Learn More link... Leaving this open for the time being in case we want to address this point in this bug.

I've filed: Bug 646788 - Add Help Topic for "Plugin Crashed" UI Bug 646792 - Add a landing page for the "Learn More" link from the plugin-crashed notification from Bug 601493.

(In reply to comment #35) > > Good question. I only just noticed that the help icon does something different > > to the Learn More link... > Leaving this open for the time being in case we want to address this point in > this bug. (In reply to comment #36) > Bug 646792 - Add a landing page for the "Learn More" link from the > plugin-crashed notification from Bug 601493. Given the followup filed, I'm closing this. If this was a wrong choice, please tell me why.

(In reply to comment #37) > (In reply to comment #35) > > > Good question. I only just noticed that the help icon does something different > > > to the Learn More link... > > Leaving this open for the time being in case we want to address this point in > > this bug. > > (In reply to comment #36) > > Bug 646792 - Add a landing page for the "Learn More" link from the > > plugin-crashed notification from Bug 601493. > > Given the followup filed, I'm closing this. If this was a wrong choice, please > tell me why. It wasn't necessarily wrong to resolve this bug in favour of a third followup, but the three things are orthogonal. 1. Clicking the ? in a crashed plugin opens missing help 2. Clicking the Learn More link in a crashed plugin infobar opens a 404 page 3. Why do 1. and 2. do different things anyway?

> 3. Why do 1. and 2. do different things anyway? Because I didn't read the Firefox code closely enough. In Firefox clicking the ? in a crashed plugin calls: openHelpLink("plugin-crashed", false); So I mapped this to opening our in App help. But a closer reading shows: function openHelpLink(aHelpTopic, aCalledFromModal) { var url = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"] .getService(Components.interfaces.nsIURLFormatter) .formatURLPref("app.support.baseURL"); url += aHelpTopic; var where = aCalledFromModal ? "window" : "tab"; openUILinkIn(url, where); } Whereas the Learn More link goes to the same URL in a different manner: let link = notification.ownerDocument.createElementNS(XULNS, "label"); link.className = "text-link"; link.setAttribute("value", gNavigatorBundle.getString("crashedpluginsMessage.learnMore")); link.href = gPluginHandler.crashReportHelpURL; .... get crashReportHelpURL() { delete this.crashReportHelpURL; let url = formatURL("app.support.baseURL", true); url += "plugin-crashed"; this.crashReportHelpURL = url; return this.crashReportHelpURL; }

(In reply to comment #39) > > 3. Why do 1. and 2. do different things anyway? > Because I didn't read the Firefox code closely enough. How fiendishly cunning of them ;-)

Cf. bug 646788 comment 2. Let's discuss either there or in m.d.a.seamonkey.