601493 - no "plugin crashed" UI

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Matthias Versen [:Matti]]

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

Comment 1

[Justin Wood (:Callek)]

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

Comment 2

[Robert Kaiser]

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

Comment 3

[Serge Gautherie (:sgautherie)]

Should this depend on bug 595810?

Comment 4

[Justin Wood (:Callek)]

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.

Comment 5

[Ian Neal]

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

Comment 6

[Robert Kaiser]

Note that this is somewhat related to bug 521159.

Comment 7

[Justin Wood (:Callek)]

Attachment: l10n only patch [checked in Comment 32]

Comment 8

[Justin Wood (:Callek)]

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.

Comment 9

[Philip Chee]

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 10

[[e-mail]]

Comment on attachment 520491 [details] [diff] [review]
l10n only patch [checked in Comment 32]

What about carbonfailureplugins?

Comment 11

[[e-mail]]

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

Comment 12

[Robert Kaiser]

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

Comment 13

[Philip Chee]

Attachment: WIP Patch v0.1 includes the L10n patch for testing purposes.

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

Comment 14

[Philip Chee]

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

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

(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!]

Comment 16

[Philip Chee]

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.

Comment 17

[Philip Chee]

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.

Comment 18

[[e-mail]]

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

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

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

Comment 20

[Philip Chee]

Attachment: Patch v1.0 Reload link always visible.

> 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 21

[Jens Hatlak (:InvisibleSmiley)]

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 22

[[e-mail]]

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 23

[[e-mail]]

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 24

[[e-mail]]

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 25

[[e-mail]]

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.

Comment 26

[[e-mail]]

(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!)

Comment 27

[Philip Chee]

(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

Comment 28

[Philip Chee]

Attachment: Patch v1.1 address review comments.

> [e-mail]      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 29

[[e-mail]]

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?

Comment 30

[[e-mail]]

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

[Philip Chee]

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 32

[Philip Chee]

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

Comment 33

[Philip Chee]

Attachment: Patch v1.2 Fix more nits [Checked in: Comment 35]

>>+              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 34

[[e-mail]]

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 35

[Philip Chee]

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.

Comment 36

[Philip Chee]

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.

Comment 37

[Justin Wood (:Callek)]

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

Comment 38

[[e-mail]]

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

Comment 39

[Philip Chee]

> 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;
  }

Comment 40

[[e-mail]]

(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 ;-)

Comment 41

[Jens Hatlak (:InvisibleSmiley)]

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