599476 - windowless plugins in object elements with data (e.g. certain Flash pages) are not painted

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Zio_Fede]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100924 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100924 Firefox/4.0b7pre

On some pages flash plugin stopped working

Reproducible: Always

Steps to Reproduce:
1.Go to http://www.youtube.com/user/tippexperience with a fresh profile
2.wait until site is loaded
3.
Actual Results:  
Instead of the video there is only a white space

Expected Results:  
The video should start

Another page with this issue is http://www.divineproject.altervista.org/.
I'll try to find the latest working build.

Comment 1

[Zio_Fede]

Latest Working hourly is 1285098901-20100921125501-81c0aef6b272-firefox-4.0b7pre.en-US.linux-i686.
First Broken hourly is 1285106168-20100921145608-8b252736a650-firefox-4.0b7pre.en-US.linux-i686.tar.bz2.

Comment 2

[Zio_Fede]

So this bug it's probably caused by https://bugzilla.mozilla.org/show_bug.cgi?id=598112 patches...

Comment 3

[Doug Turner (:dougt)]

anyone else seeing this?

Comment 4

[Matthew Turnbull [Bluefang]]

I see it on Vimeo and YTMND
http://vimeo.com/14980662


I can conform the regression range insofar that the Linux x86_64 2010-09-21 nightly works and the 2010-09-22 nightly does not.

Comment 5

[Stephen Moehle]

I see this with Mozilla/5.0 (X11; Linux x86_64; rv:2.0b7pre) Gecko/20100101 Firefox/4.0b7pre (self-built) on Fedora 13 with both Flash 10.1.85.3 (using nspluginwrapper) and 10.2.161.22, the 64-bit preview.

For me, this issue affects the the Moogalover player used by Vimeo (http://vimeo.com). YouTube videos work OK.

If I set dom.ipc.plugins.enabled to false, the Flash videos appear, though on some sites, i.e. http://www.cartoonbrew.com/, embedded Vimeo videos show as black with a play button on it. On these sites, playing the video gives audio and the player's controls, but the video itself remains black.

Comment 6

[TimoT]

I see the same on a windows build that has patches from bug 596451.

Backing out

-  if (RUNNING != mRunning)
-    return NS_OK;
-

from bug 598112 fixes the issue.

Comment 7

[Oleg Romashin (:romaxa)]

Yep, I see it is reproducible, 

> -  if (RUNNING != mRunning)
> -    return NS_OK;
> -
> 
removing this will disable plugin layers implementation....


(In reply to comment #6)
> I see the same on a windows build that has patches from bug 596451.

interesting why this is visible windows build, because plugin layers should not be available on windows

Comment 8

[Zio_Fede]

(In reply to comment #5)
> I see this with Mozilla/5.0 (X11; Linux x86_64; rv:2.0b7pre) Gecko/20100101
> Firefox/4.0b7pre (self-built) on Fedora 13 with both Flash 10.1.85.3 (using
> nspluginwrapper) and 10.2.161.22, the 64-bit preview.
> 
> For me, this issue affects the the Moogalover player used by Vimeo
> (http://vimeo.com). YouTube videos work OK.
> 
> If I set dom.ipc.plugins.enabled to false, the Flash videos appear, though on
> some sites, i.e. http://www.cartoonbrew.com/, embedded Vimeo videos show as
> black with a play button on it. On these sites, playing the video gives audio
> and the player's controls, but the video itself remains black.

Vimeo black playback happens even in the latest beta, so it's probably another regression...

Comment 9

[Oleg Romashin (:romaxa)]

Ok, I found where is the problem,
ObjectFrame and PluginInstance created and then PluginInstance immediately destroyed.
After that new PluginInstance created with the same ObjectFrame but CallSetWindow not called and surface not initialized.

I'll fix it soon

Comment 10

[Oleg Romashin (:romaxa)]

Easy way to get that working is press CTRL + and CTRL+0 (force reflow and setWindow call)

Comment 11

[Zio_Fede]

I cam make it work by changing the zoom, but animations freeze by right-clicking them.

Comment 12

[Zio_Fede]

(In reply to comment #8)
> (In reply to comment #5)
> > I see this with Mozilla/5.0 (X11; Linux x86_64; rv:2.0b7pre) Gecko/20100101
> > Firefox/4.0b7pre (self-built) on Fedora 13 with both Flash 10.1.85.3 (using
> > nspluginwrapper) and 10.2.161.22, the 64-bit preview.
> > 
> > For me, this issue affects the the Moogalover player used by Vimeo
> > (http://vimeo.com). YouTube videos work OK.
> > 
> > If I set dom.ipc.plugins.enabled to false, the Flash videos appear, though on
> > some sites, i.e. http://www.cartoonbrew.com/, embedded Vimeo videos show as
> > black with a play button on it. On these sites, playing the video gives audio
> > and the player's controls, but the video itself remains black.
> 
> Vimeo black playback happens even in the latest beta, so it's probably another
> regression...

https://bugzilla.mozilla.org/show_bug.cgi?id=596454

Comment 13

[Aakash Desai [:aakashd]]

I see this quite a bit on newgrounds.com/movies and www.youtube.com on friday's nightly build of Fennec on maemo. I'm getting either a blank space or the plugin disabled message.

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Track call set window after SetInstance call

Variant 1, track destroy calls, and call setWindow in BuildLayer if new Instance was installed

Comment 15

[Oleg Romashin (:romaxa)]

Attachment: another version, check the same what DrawWithXlib cheking

in non-layers plugin mode, we have SetWindow call coming from DrawWithXlib call because of offset not equals to mPluginWindow->x/y.

This patch is trying to do the same thing

Comment 16

[Oleg Romashin (:romaxa)]

Comment on attachment 478673 [details] [diff] [review]
Track call set window  after SetInstance call

roc is on vacation

Comment 17

[Oleg Romashin (:romaxa)]

Comment on attachment 478675 [details] [diff] [review]
another version, check the  same what DrawWithXlib cheking

roc is on vacation, benjamin I need your feedback

Comment 18

[Benjamin Smedberg]

Comment on attachment 478673 [details] [diff] [review]
Track call set window  after SetInstance call

This one seems clearer to me. We should really get some sort of peer to review this: karlt/bz/josh?

Comment 19

[Markus Popp]

There are also issues with Google Maps, as reported in bug 599492 and bug 599493.

Also http://www.norc.at/street-view/ is having Flash related issues.

May they be related to this bug?

Comment 20

[Daniel Holbert [:dholbert]]

Also seems to affect http://www.cnn.com/video/

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 478675 [details] [diff] [review]
another version, check the  same what DrawWithXlib cheking

With non-layer X11 plugins, the first SetWindow always arrives before the first
paint, even with 0 offset, because the visual (and colormap) are 0 until
the first paint.

With non-layer GDI plugins, the first SetWindow always arrives because the HDC
is null until the first paint.

Comment 22

[Karl Tomlinson (:karlt)]

It should be possible to write a simple reftest based on
data:text/html,<object width="200" height="200" data="data:application/x-test,0" drawmode="solid" color="ff00ff00"></object>

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 478673 [details] [diff] [review]
Track call set window  after SetInstance call

Because the plugin is painting asynchronously, I think it would be better to
tell the plugin its size and tell the host we are using layers as soon as possible, rather than at the point where we try to use what it would paint.

This looks like a good place for that,
http://hg.mozilla.org/mozilla-central/annotate/5a2012482a63/modules/plugin/base/src/nsPluginStreamListenerPeer.cpp#l598
which would be more consistent with the currently working
nsObjectFrame::Instantiate(const char* aMimeType, nsIURI* aURI) behavior (as well as more consistent with windowed plugin behavior).

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 478673 [details] [diff] [review]
Track call set window  after SetInstance call

This issue here is not that SetInstance is replacing an existing PluginInstance with a new one (which would fire an assertion), but that these object elements use nsObjectFrame::Instantiate(nsIChannel* aChannel, nsIStreamListener** aStreamListener), which, unlike nsObjectFrame::Instantiate(const char* aMimeType, nsIURI* aURI), does not call AsyncSetWindow.

The appropriate fix is to call AsyncSetWindow as indicated in comment 23.

Comment 25

[Oleg Romashin (:romaxa)]

Attachment: part 2: Fixd according karl comment

Comment 26

[Benjamin Smedberg]

We really need an automated test for this. Can we do it with a reftest?

Comment 28

[Karl Tomlinson (:karlt)]

(In reply to comment #26)
> We really need an automated test for this. Can we do it with a reftest?

Yes.  Compare a solid green div with

<object width="200" height="100"
data="data:application/x-test,0" drawmode="solid" color="ff00ff00"></object>

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 480136 [details] [diff] [review]
part 2: Fixd according karl comment

This part is good, but we can't use it until UseAsyncPainting knows about mozilla.plugins.use_layers.  I assume testing that pref can be moved to nsNPAPIPluginInstance.

Comment 30

[Oleg Romashin (:romaxa)]

Attachment: part 1: prefs part

Comment 31

[Oleg Romashin (:romaxa)]

Comment on attachment 480136 [details] [diff] [review]
part 2: Fixd according karl comment

I think these two patches make sense now.

Comment 32

[Oleg Romashin (:romaxa)]

Attachment: reftest

Comment 33

[Karl Tomlinson (:karlt)]

Comment on attachment 480364 [details] [diff] [review]
part 1: prefs part

>   PRBool UseLayers()
>   {
>-    return (mUsePluginLayers &&
>+    PRBool useAsyncRendering = PR_FALSE;

No need to initialize useAsyncRendering as there is an NS_SUCCEEDED check.

>+    return (NS_SUCCEEDED(mInstance->UseAsyncPainting(&useAsyncRendering)) &&
>+            useAsyncRendering &&
>             (!mPluginWindow ||
>              mPluginWindow->type == NPWindowTypeDrawable));

Add a null check for mInstance.

>+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+  if (prefs) {
>+    PRBool useLayersPref = mUsePluginLayersPref;

No need to initialize useLayersPref.
It is only an outparam for GetBoolPref().

Comment 34

[Karl Tomlinson (:karlt)]

Comment on attachment 480367 [details] [diff] [review]
reftest

Thanks!

Comment 35

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/bb18e81ea0f8