922349 - Removing iframe with plugin content breaks doorhanger

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[:Margaret Leibovic]

Attachment: Screen Shot 2013-09-30 at 3.52.33 PM.PNG

See screenshot.

STR:
1) Visit http://www.kenneth-li.com/
2) Click the "View" button on the "Dibbs" project
3) Click outside the video overlay to dismiss it
4) Click on lego notification icon

Comment 1

[Georg Fritzsche [:gfritzsche]]

This looks like somthing similar to bug 916542.

Comment 2

[Georg Fritzsche [:gfritzsche]]

This site adds an iframe with plugin content and removes the iframe again when clicking outside it.
nsObjectLoadingContent does fire "PluginRemoved", but the front-end doesn't receive it and hence never hides the notification.

I guess this might be an issue here due to the subdocument/iframe going away, which is the document we use nsContentUtils::DispatchTrustedEvent() on via [1]/[2].

[1] http://hg.mozilla.org/mozilla-central/annotate/0d9f5e4bd5b8/content/base/src/nsObjectLoadingContent.cpp#l713
[2] http://hg.mozilla.org/mozilla-central/annotate/0d9f5e4bd5b8/content/base/src/nsObjectLoadingContent.cpp#l254

Comment 3

[Benjamin Smedberg]

Is there a browser console error? This is supposed to be prevented by the patch from bug 889788, but if there were a JS exception that would leave the doorhanger in this completely broken-looking state.

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Is there a browser console error?

Nothing related in the browser console.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Attachment: Test-case

Comment 6

[Georg Fritzsche [:gfritzsche]]

Took a second look - for the simple test-case, we hit nsObjectLoadingContent::UnbindFromTree() two times, scheduling two nsSimplePluginEvent("PluginRemoved"): for the first thisContent->GetCurrentDoc() is non-null, for the second it's null.
gPluginHandler.handleEvent() isn't hit at all for me after the initial "PluginBindingAttached".

Comment 7

[Georg Fritzsche [:gfritzsche]]

Also - not too surprisingly - the second call to DispatchTrustedEvent(), with mTarget/mDocument being 0, gets a NS_ERROR_ILLEGAL_VALUE while the first one is fine.
I guess i'll trace the event delivery down if there is no other input/ideas.

Comment 8

[Benjamin Smedberg]

Ignore the PluginRemoved stuff for now. Let's just focus on dismissing the doorhanger if we do end up in this broken state. Long-term, we actually want to keep the plugin listed in the doorhanger anyway even if it gets removed.

Comment 9

[John Schoenick [:johns]]

We should probably fix the PluginRemoved stuff as part of bug 920527. It sounds like we need to find out why UnbindFromTree is being called with a null document, and if there's a way to find the parent docshell to always fire events at the topmost document.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Alright, moved those concerns to bug 920527.

Comment 11

[Georg Fritzsche [:gfritzsche]]

So per bug 926859 i currently don't see a good way to cancel or hide the popup from the urlbarBinding constructor or the PopupNotification "showing" notification.

Setting |PopupNotifications.panel.hidden=true| from PH_makeCenterActions() works, but seems rather crude & fragile.
Gavin, do you maybe have a better suggestion here that would be in scope for Fx26?

Comment 12

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

As mentioned on IRC, I suspect a setTimeout(0) would fix that. We should file a bug to investigate firing the popupnotification callbacks asynchronously.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Attachment: Remove the notification when there are no plugins

https://tbpl.mozilla.org/?tree=Try&rev=8cdde5a4b198

Gavin, given that you already know the context can you review this?

Comment 14

[Georg Fritzsche [:gfritzsche]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #12)
> We should
> file a bug to investigate firing the popupnotification callbacks
> asynchronously.

Filed bug 927177.

Comment 15

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

Comment on attachment 817499 [details] [diff] [review]
Remove the notification when there are no plugins

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

>+    if (centerActions.length == 0) {
>+      // TODO: this is a temporary band-aid to avoid broken doorhangers
>+      // until bug 926605 is landed.
>+      notification.options.centerActions = [];
>+      setTimeout(() => PopupNotifications.remove(notification), 0);
>+      return;

Why do you need to set options.centerActions = [] before calling remove()?

Comment 16

[Georg Fritzsche [:gfritzsche]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #15)
> Why do you need to set options.centerActions = [] before calling remove()?

The popup code in urlbarBindings.xml expects this to be set and i added an early-return here before it gets set later in PH_makeCenterActions().

Comment 17

[Georg Fritzsche [:gfritzsche]]

Minor test-fixup required:
https://tbpl.mozilla.org/?tree=Try&rev=ec7e4981bc96

Comment 18

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e337dc39645f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e337dc39645f

Comment 20

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 817499 [details] [diff] [review]
Remove the notification when there are no plugins

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play redesign
User impact if declined: Users get so see a broken doorhanger after the last plugin was removed from the page (when plugin was in an iframe as is common for youtube/vimeo/... embedding).
Testing completed (on m-c, etc.): On m-c, pending verification.
Risk to taking this patch (and alternatives if risky): Low-risk - worst-case would probably be the doorhanger not opening in some edge case.
String or IDL/UUID changes made by this patch: None.

Comment 21

[Paul Silaghi, QA [:pauly]]

Tested a nightly after the fix.
After removing the frame, the CTP doorhanger remains visible. Clicking then on the doorhanger, makes it disappear. Is this ok ?

Comment 22

[Georg Fritzsche [:gfritzsche]]

That is intentional, we decided to go with that for now until we fix bug 926605.

Comment 23

[Paul Silaghi, QA [:pauly]]

Verified fixed based on comment 22 on 27.0a1 (2013-10-20) Win 7, Ubuntu 13.04 and Mac OS X 10.8.4

Comment 24

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b776d213187

Comment 25

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.1; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on latest Aurora 26.0a2 (buildID: 20131024004004).