546502 - Refreshing on toolbar for crash plugin UI doesn't remove notification bar

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tony Chung [:tchung]]

Attachment: Crash plugin notification bar screenshot

If you refresh a crashed plugin with the notification window open using the toolbar Refresh button, it will not hide the plugin UI crash notification bar.

Repro:
1) run nightly trunk: 
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100216 Minefield/3.7a2pre
2) set ipc to true
3) open a flash plugin and force kill the process.   I used URL: http://www.vh1.com/video/frank-the-entertainer-in-a-basement-affair/full-episodes/meet-the-rest-of-the-marescas/1631827/playlist.jhtml
4) Notice the notification crash UI drop down ("The Shockwave Flash plugin has crashed")
5) now click the "refresh" button on the toolbar menu.   
6) Verify the page refreshes with the plugin reloading, but the notification bar doesn't hide.

However, clicking the "reload page" button on the notification will hide the notification bar.

* See screenshot

Expected:
- Refreshing via toolbar will hide the notification bar.

Comment 1

[[not reading bugmail]]

Looks like there needs to be some code similar to when you dismiss other notification bars like: installing plugin's or adding lwthemes (and don't have whitelisted getpersonas.com) where clicking on a button also removes the bar after it is no longer needed.

Comment 2

[Maini Marco]

I can't see any notification toolbar in today's builds. It is possible that notification bar was removed. In this case, also this bug is fixed.

Comment 3

[u88484]

Something broke here.  Open gmail and two youtube videos.  Focus one of the youtube videos.  Kill the m-r.exe.  The notification bar does now show up on the other youtube video's tab but does show up on the gmail tab.

Seems like the notification bar is only showing for other sites.

Comment 4

[u88484]

(In reply to comment #3)
> Something broke here.  Open gmail and two youtube videos.  Focus one of the
> youtube videos.  Kill the m-r.exe.  The notification bar does now show up on
> the other youtube video's tab but does show up on the gmail tab.
> 
> Seems like the notification bar is only showing for other sites.

Ignore this.  I misunderstood when the notification bar is supposed to show and i think Maini did also.

"the notification bar on pages where the plugin is too small to see the plugin-crashed UI directly"

Maini, try on sites that has very small plugin content.  Gmail is one such site.

Comment 5

[Maini Marco]

It seems that Gmail page contains Flash plugin. When you kill mozilla-runtime the plugin-crashed UI appears in youtube video's tab and the notification bar in Gmail's tab.
If you click reload in plugin-crashed UI video restarts, but plugin in Gmail's page isn't reloaded. Otherwise, if you click notification bar in Gmail's page plugin restarts, but youtube video's tab continues to show crashed UI.

This seems like a correct behaviour in my opinion.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

The specific problem here is that the notification bar was only being removed when clicking its "reload page" button. Other methods of reloading the page don't trigger the button's callback, and since the location doesn't change the notification bar doesn't remove itself (as would happen if clicking a link in the page).

Adding an unload handler seems the easiest way to notice when things were reloaded. [I do this on window.top, since it would be complicated to track a tab full of frames, and the UX would be confusing.]

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 438024 [details] [diff] [review]
Patch v.1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+      // Remove the notfication when the page is reloaded.
>+      doc.defaultView.top.addEventListener("unload", function() {
>+        notificationBox.removeNotification(notification); }, false);

I prefer:

doc.defaultView.top.addEventListener("unload", function() {
  notificationBox.removeNotification(notification);
}, false);

Comment 8

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/687d99d25910

Comment 9

[Dão Gottwald [:dao]]

The unload handler is going to break the bfcache, AFAIK; pagehide should be used instead.

Comment 10

[Justin Dolske [:Dolske]]

(In reply to comment #9)
> The unload handler is going to break the bfcache

That actually seems desirable; the handler is only added when the page has a (small) crashed plugin, and I don't see what we'd want to cache that state.

Comment 11

[Dão Gottwald [:dao]]

Shouldn't that be handled at a lower level? How was this being dealt with without this patch? It sounds like the wrong fix to a different problem.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

It wasn't being dealt with without this patch. It's an unintended side effect of this patch that turned out to be beneficial. It could also be handled at a lower level, but I don't think we really need to worry about it. Might be worth adding a comment, though.

Comment 13

[LegNeato]

Comment on attachment 438024 [details] [diff] [review]
Patch v.1

a=LegNeato for 1.9.2.4. Please land on both mozilla-1.9.2 default and
GECKO1924_20100413_RELBRANCH

Comment 14

[Justin Dolske [:Dolske]]

Pushed to 1.9.2...
relbranch: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2de16fe1b3af
default: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/08f05330eb8b

Comment 15

[Al Billings [:abillings - ex-MoCo]]

I'm not sure how to verify this since we don't use a notification bar anymore for OOPsies. Tony?

Comment 16

[:Gavin Sharp [email: [e-mail]]]

We still show a notification bar for plugin crashes, but only if the plugin object is too small to show the in-page UI.