Closed Bug 922349 Opened 9 years ago Closed 9 years ago

Removing iframe with plugin content breaks doorhanger

Categories

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

defect

Tracking

(firefox26+ verified, firefox27+ verified)

VERIFIED FIXED
mozilla27
Tracking Status
firefox26 + verified
firefox27 + verified

People

(Reporter: Margaret, Assigned: gfritzsche)

References

Details

Attachments

(3 files)

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
Component: General → Plug-ins
Product: Firefox → Core
This looks like somthing similar to bug 916542.
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
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
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.
Blocks: 889788
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Is there a browser console error?

Nothing related in the browser console.
Attached file Test-case
Summary: Click-to-play plugin notification has empty spaces → Removing iframe with plugin content breaks doorhanger
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".
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.
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.
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.
Blocks: 920527
Alright, moved those concerns to bug 920527.
Assignee: nobody → georg.fritzsche
Depends on: 926859
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?
Flags: needinfo?(gavin.sharp)
As mentioned on IRC, I suspect a setTimeout(0) would fix that. We should file a bug to investigate firing the popupnotification callbacks asynchronously.
Flags: needinfo?(gavin.sharp)
https://tbpl.mozilla.org/?tree=Try&rev=8cdde5a4b198

Gavin, given that you already know the context can you review this?
Attachment #817499 - Flags: review?(gavin.sharp)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)
> We should
> file a bug to investigate firing the popupnotification callbacks
> asynchronously.

Filed bug 927177.
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()?
Attachment #817499 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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().
No longer depends on: 926859
https://hg.mozilla.org/mozilla-central/rev/e337dc39645f
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Keywords: verifyme
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.
Attachment #817499 - Flags: approval-mozilla-aurora?
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 ?
Flags: needinfo?(georg.fritzsche)
That is intentional, we decided to go with that for now until we fix bug 926605.
Flags: needinfo?(georg.fritzsche)
Verified fixed based on comment 22 on 27.0a1 (2013-10-20) Win 7, Ubuntu 13.04 and Mac OS X 10.8.4
Attachment #817499 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.