Closed Bug 697651 Opened 13 years ago Closed 12 years ago

With patches in bug 90268, display:none autoplay youtube video embeds now autoplay

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Version:
Other Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: karlt, Unassigned)

References

(
URL
)

Details

I notice that news.google.co.nz is using display:none to prevent embedded Flash autoplay youtube videos from playing.

The patches in bug 90268 currently turn http://news.google.co.nz/ into an election add speech that repeats every time the page auto-refreshes.

A testcase showing the behavior change is
data:text/html,<embed src="http://www.youtube.com/v/I3O8DZrK5eQ&amp;autoplay=1"%20type="application/x-shockwave-flash"%20wmode="transparent"%20style="display:none">
How does it prevent those videos from playing in other browsers?
Still happens with
>@@ -167,1 +167,1 @@ PluginInstanceChild::PluginInstanceChild
>-    , mHasPainted(false)
>+    , mHasPainted(true)

so doesn't seem to be caused by the bogus paint.

In fact, I don't see either mPluginIface->setwindow() or PluginInstanceChild::ShowPluginFrame() getting called at all.
Would be good to check what it actually serves to Chrome and other browsers... it might be different.
I don't see anything obviously different served to Chromium 15.0.874.106 claiming to be Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.2 (KHTML, like Gecko) Chrome/15.0.874.106 Safari/535.2

The data URL testcase does not play in Chromium.
(In reply to Karl Tomlinson (:karlt) from comment #4)
> The data URL testcase does not play in Chromium.

Interesting. We need to figure out how Chromium treats the plugin differently there.
Attachment 72926 [details] in Chromium restarts the animation when changing the embed style between display:block and display:inline, but not when changing an ancestor between block and inline.

Attachment 570172 [details] in Chromium restarts the animation when changing the ancestor style to display:none and back, but doesn't not restart the animation when changing ancestor position or overflow style.
(In reply to Karl Tomlinson (:karlt) from comment #2)
> Still happens with
> >@@ -167,1 +167,1 @@ PluginInstanceChild::PluginInstanceChild
> >-    , mHasPainted(false)
> >+    , mHasPainted(true)
> 
> so doesn't seem to be caused by the bogus paint.
> 
> In fact, I don't see either mPluginIface->setwindow() or
> PluginInstanceChild::ShowPluginFrame() getting called at all.

Maybe as a hack you could try disabling various plugin entry points to see which of our NPAPI calls to the plugin are required for the video to play.
The other thing to check is whether loading a simple "play some sounds" Flash object as display:none does play in Chrome.
Chromium calls (*destroy)() when the ancestor becomes display:none and (*newp)() when display is restored.
Errr ... so in general Webkit doesn't instantiate display:none plugins? What about plugins that aren't in the document? What does the spec say about this? Is the whole premise of bug 90268 wrong???!

I presume we could tweak 90268 for whatever policy we need. Even if we have to not instantiate for display:none, 90268 gives us a better implementation model. Still!
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Maybe as a hack you could try disabling various plugin entry points to see
> which of our NPAPI calls to the plugin are required for the video to play.

The audio plays with just newp, destroy, newstream, destroystream, writeready, and write.

I guess we could choose to not respond to NPN_GetURL() (and friends) while the plugin is display:none, but it might be easier and safer to follow the webkit approach.
We need to coordinate this with standards groups. The spec clearly says:

http://www.whatwg.org/specs/web-apps/current-work/#the-object-element
The above algorithm is independent of CSS properties (including 'display', 'overflow', and 'visibility'). For example, it runs even if the element is hidden with a 'display:none' CSS style, and does not run again if the element's visibility changes.

If other browsers aren't doing that, and the Web depends on not doing that, we need to get the spec changed.
IE9, Chrome, and Opera all behave the same as trunk Firefox on the testcase in comment #0.

One problem is that it's hard to tell if the Flash object itself is doing something weird --- it could be running JS to inspect its DOM element, or even doing user-agent sniffing (or maybe you disabled that in comment #11?) and changing its behavior.

Maybe we need to modify our test plugin to record when it gets instantiated, and test that.
Just an FYI, the patch for 90268 adds two capabilities:

1) Allow DOM node re-parenting without restarting the plugin.
2) Allow display:none plugins to run.

It's easy enough to make my patch stop doing #2, we basically just move instantiation to nsObjectLoadingContent::HasNewFrame.

Karl, roc: Are you guys going to continue to try to determine the desired behavior or should I find an owner for this bug (possibly myself)?
HasNewFrame will get called when reparenting DOM nodes or when recreating frames.  I assume the idea is that it won't instantiate if there is already an instance?
(In reply to Boris Zbarsky (:bz) from comment #15)
> HasNewFrame will get called when reparenting DOM nodes or when recreating
> frames.  I assume the idea is that it won't instantiate if there is already
> an instance?

Right, I was just giving a basic summary of the change.
I think we are going to strike problems if we let display:none plugins run, so changing the patch to stop doing #2 sounds good, at least as a safer first step.

I've feel I've finished investigation of the cause of the bug, and Josh knows the new code so could probably turn off 2 faster than anyone else.

Let me know if you'd like me to check Opera behavior too.
(In reply to Josh Aas (Mozilla Corporation) from comment #14)
> Just an FYI, the patch for 90268 adds two capabilities:
> 
> 1) Allow DOM node re-parenting without restarting the plugin.
> 2) Allow display:none plugins to run.
> 
> It's easy enough to make my patch stop doing #2, we basically just move
> instantiation to nsObjectLoadingContent::HasNewFrame.
> 
> Karl, roc: Are you guys going to continue to try to determine the desired
> behavior or should I find an owner for this bug (possibly myself)?

I think we should go ahead and finish 90268 with some change to not instantiate display:none plugins. That simply makes it a smaller change away from our trunk behavior, which is good.
To help figure out what the behavior should be, I wrote a test here:
http://people.mozilla.com/~roc/object_instantiation_test.html

Test 1: Does anything play when the page loads?
Test 2: Does clicking on "Show" start playback?
Test 3: Does clicking on "Hide" stop audio playback?
Test 4: Does clicking on "Show and hide" do anything?
Test 5: Does clicking on "Show and hide with flush" do anything?
Test 6: Does clicking on "Show and hide with alert" do anything?

Results on Windows 7:
               1   2    3    4        5          6
IE9:           No  Yes  No   No       Sometimes  Plays
Chrome 16 dev: No  Yes  Yes  No       No         Relayout
Firefox trunk: No  Yes  Yes  No       No         Plays
Opera 11.52:   No  Yes  Yes  Flicker  Flicker    Plays

"Sometimes" -> In this case, IE9 sometimes shows the video for a moment and starts playing the audio.
"Flicker" -> In these cases, Opera shows the video and plays the audio for a moment.
"Plays" -> In these cases, the browser plays the video and audio while the alert is up.
"Relayout" -> In this case, Chrome adjusts the layout to make space for the video while the alert is up, but doesn't play anything.
I think our trunk behavior makes most sense here. Test 6 probably doesn't matter though so that matches Chrome on the tests that matter.

The best way to do this might be to instantiate/uninstantiate a plugin off an event after any nsObjectFrame construction or destruction. That event would check for the existence of an nsObjectFrame for the plugin to decide whether to instantiate or not.
We can't instantiate off of an event, or at least we can't rely on only that. Some pages add the plugin to the document and immediately try to call into it from js. Making this work is critical. That fails if we don't try to sync instantiate from the plugin node's PostCreate method (I forget the exact function names, it happens from nsHTMLPluginObjElementSH::SetupProtoChain, which calls nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe, which attempt sync instantiation). The current patch attempts instantiation that way first, but also creates an instantiation event which does nothing if the instance already started.
(In reply to Josh Aas (Mozilla Corporation) from comment #21)
> We can't instantiate off of an event, or at least we can't rely on only
> that. Some pages add the plugin to the document and immediately try to call
> into it from js. Making this work is critical.

Yes. That needs to be added to the spec too. But I think it's totally OK to run the "try to instantiate a plugin" algorithm synchronously in that case. You will need to flush style so the display:none check is up to date.
WebKit made display:none plug-in not get instantiated <https://bugs.webkit.org/show_bug.cgi?id=15081>, which broke Gears <https://bugs.webkit.org/show_bug.cgi?id=24215>, and is a change that some people want to undo <https://bugs.webkit.org/show_bug.cgi?id=45049>.
Please take that up on the WHATWG thread:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-November/033732.html

Gears is dead, and I'm confused about how Web sites could depend on display:none plugins being instantiated, in general, since no browser currently does this (and Firefox at least has *never* done it).
I'm not arguing either way (if anything, I'd prefer display:none to kill plugins so that I can block them from CSS). Just wanted you to be aware of WebKit's movements in this area.
I will modify the patch in bug 90268 to not instantiate plugins until they have frames. Hopefully I can do that by the end of the week.
hi folks,

what will be the status if I intentionally use 'display:none' to avoid the layout space reservation when use of plug-ins.

for example,keeping background music(I don't want the graphics here) for my website with flash plugin styled as 'display:none'.

(still possible with 'visibility:hidden;width:0;height:0;position:absolute',
but 'visibility:hidden;width:0;height:0;position:absolute' != 'display:none')

regards,
vicky
You should only need width:0;height:0 in practice.
yes, a web author do the layout disabling with 'width:0;height:0;' css property. but some browsers like netfront & ANT galio supports 'display:none' plugin instantiating, so web applications developed using those will not compatible with webkit. i think, w3c spec too says plugin loading shouldn't be based on css properties.

regards,
vicky.
Karl, is this reproducible now that bug 90268 has landed?
This was fixed in bug 90268 before that landed.
Tested and can't reproduce now.

Bug 723379 sounds similar, but may be something quite different.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
HTML5 specification also requires the display:none plugin to run.

http://www.w3.org/TR/2011/WD-html5-20110525/the-iframe-element.html#the-object-element

Also CE browsers like ANT & Netfront & general browser & Opera allows display:none plugins to run.
The latest HTML5 draft says that display:none plugins should not run.
http://dev.w3.org/html5/spec/single-page.html#the-object-element

> ... if the element is not being rendered, then jump to the last step in the
> overall set of steps (fallback).

http://dev.w3.org/html5/spec/single-page.html#being-rendered
vicky, you're reading a draft that's more than a year old a this point.  It's _very_ out of date; a lot of problems with the spec have been fixed since then....
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.