Removing iframe with plugin content breaks doorhanger

VERIFIED FIXED in Firefox 26

Status

()

Core
Plug-ins
P2
normal
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: Margaret, Assigned: gfritzsche)

Tracking

(Blocks: 1 bug)

Trunk
mozilla27
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox26+ verified, firefox27+ verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 812276 [details]
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
(Assignee)

Updated

5 years ago
Component: General → Plug-ins
Product: Firefox → Core
(Assignee)

Comment 1

5 years ago
This looks like somthing similar to bug 916542.
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Updated

5 years ago
status-firefox26: --- → affected
status-firefox27: --- → affected
tracking-firefox26: --- → ?
tracking-firefox27: --- → ?

Comment 3

5 years ago
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.

Updated

5 years ago
Blocks: 889788
(Assignee)

Comment 4

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

Nothing related in the browser console.
(Assignee)

Comment 5

5 years ago
Created attachment 812774 [details]
Test-case
(Assignee)

Updated

5 years ago
Summary: Click-to-play plugin notification has empty spaces → Removing iframe with plugin content breaks doorhanger
(Assignee)

Comment 6

5 years ago
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".
(Assignee)

Comment 7

5 years ago
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

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 920527
(Assignee)

Comment 10

5 years ago
Alright, moved those concerns to bug 920527.
(Assignee)

Updated

5 years ago
Assignee: nobody → georg.fritzsche
tracking-firefox26: ? → +
tracking-firefox27: ? → +
(Assignee)

Updated

4 years ago
Depends on: 926859
(Assignee)

Comment 11

4 years ago
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)
(Assignee)

Comment 13

4 years ago
Created attachment 817499 [details] [diff] [review]
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?
Attachment #817499 - Flags: review?(gavin.sharp)
(Assignee)

Comment 14

4 years ago
(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+
(Assignee)

Comment 16

4 years ago
(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
Last Resolved: 4 years ago
status-firefox27: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(Assignee)

Updated

4 years ago
Keywords: verifyme
(Assignee)

Comment 20

4 years ago
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)
(Assignee)

Comment 22

4 years ago
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
status-firefox27: fixed → verified
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
status-firefox26: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.