Closed Bug 546502 Opened 15 years ago Closed 15 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.7a5
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: tchung, Assigned: Dolske)

References

()

Details

Attachments

(1 file, 1 obsolete file)

Attached image Crash plugin notification bar screenshot (obsolete) (deleted) —
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.
Blocks: OOPP
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.
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.
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.
(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.
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.
Attached patch Patch v.1 (deleted) — Splinter Review
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.]
Assignee: nobody → dolske
Attachment #427199 - Attachment is obsolete: true
Attachment #438024 - Flags: review?(gavin.sharp)
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);
Attachment #438024 - Flags: review?(gavin.sharp) → review+
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Attachment #438024 - Flags: approval1.9.2.4?
The unload handler is going to break the bfcache, AFAIK; pagehide should be used instead.
Component: Plug-ins → General
Product: Core → Firefox
QA Contact: plugins → general
Target Milestone: mozilla1.9.3a5 → ---
OS: Windows 7 → All
Hardware: x86 → All
(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.
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.
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 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
Attachment #438024 - Flags: approval1.9.2.4? → approval1.9.2.4+
Target Milestone: --- → Firefox 3.7a5
I'm not sure how to verify this since we don't use a notification bar anymore for OOPsies. Tony?
We still show a notification bar for plugin crashes, but only if the plugin object is too small to show the in-page UI.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: