Last Comment Bug 697651 - With patches in bug 90268, display:none autoplay youtube video embeds now autoplay
: With patches in bug 90268, display:none autoplay youtube video embeds now aut...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Other Branch
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
data:text/html,<embed src="http://www...
Depends on:
Blocks: 90268
  Show dependency treegraph
 
Reported: 2011-10-26 20:26 PDT by Karl Tomlinson (:karlt)
Modified: 2012-07-12 05:28 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Karl Tomlinson (:karlt) 2011-10-26 20:26:09 PDT
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">
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-26 21:00:46 PDT
How does it prevent those videos from playing in other browsers?
Comment 2 Karl Tomlinson (:karlt) 2011-10-26 21:31:50 PDT
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.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-26 21:50:06 PDT
Would be good to check what it actually serves to Chrome and other browsers... it might be different.
Comment 4 Karl Tomlinson (:karlt) 2011-10-26 22:26:09 PDT
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.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-26 22:33:05 PDT
(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.
Comment 6 Karl Tomlinson (:karlt) 2011-10-27 21:07:57 PDT
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.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-27 21:14:53 PDT
(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.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-27 21:15:59 PDT
The other thing to check is whether loading a simple "play some sounds" Flash object as display:none does play in Chrome.
Comment 9 Karl Tomlinson (:karlt) 2011-10-27 21:51:20 PDT
Chromium calls (*destroy)() when the ancestor becomes display:none and (*newp)() when display is restored.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-27 22:07:49 PDT
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!
Comment 11 Karl Tomlinson (:karlt) 2011-10-27 22:57:20 PDT
(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.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-28 01:42:15 PDT
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.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-28 01:54:05 PDT
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.
Comment 14 Josh Aas 2011-11-01 08:35:21 PDT
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)?
Comment 15 Boris Zbarsky [:bz] (TPAC) 2011-11-01 09:06:17 PDT
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?
Comment 16 Josh Aas 2011-11-01 09:07:55 PDT
(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.
Comment 17 Karl Tomlinson (:karlt) 2011-11-01 15:11:50 PDT
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.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-01 15:43:39 PDT
(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.
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-01 16:02:27 PDT
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.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-01 16:15:15 PDT
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.
Comment 21 Josh Aas 2011-11-01 17:41:21 PDT
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.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-01 20:53:21 PDT
(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.
Comment 23 Simon Fraser 2011-11-02 16:44:16 PDT
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>.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-02 16:51:45 PDT
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).
Comment 25 Simon Fraser 2011-11-02 18:20:16 PDT
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.
Comment 26 Josh Aas 2011-11-02 19:03:59 PDT
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.
Comment 27 vicky 2011-11-03 02:38:42 PDT
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
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-03 04:05:03 PDT
You should only need width:0;height:0 in practice.
Comment 29 vicky 2011-11-04 02:03:44 PDT
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.
Comment 30 Ryan VanderMeulen [:RyanVM] 2012-02-02 10:29:22 PST
Karl, is this reproducible now that bug 90268 has landed?
Comment 31 Karl Tomlinson (:karlt) 2012-02-02 12:34:35 PST
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.
Comment 32 vicky 2012-07-12 03:01:02 PDT
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.
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-12 03:11:21 PDT
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
Comment 34 Boris Zbarsky [:bz] (TPAC) 2012-07-12 05:28:45 PDT
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....

Note You need to log in before you can comment on or make changes to this bug.