604420 - Some Flash videos no longer play in presence of JavaScript-based content policies

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Wladimir Palant]

Attachment: Minimal content policy for testing

Some Flash videos will no longer play in Minefield if Adblock Plus is installed, e.g. the video on http://edition.cnn.com/ - clicking "Click to play" button makes the image disappear but the Flash video that should display instead doesn't show up. Same happens with RuTube videos like http://rutube.ru/tracks/3674261.html - here the video should show up on page load but doesn't.

This regressed in Minefield between 20100913 and 20100914 (reproduced on Windows 7 x64). Adblock Plus doesn't need to be installed - it is also reproducible on a clean profile with the minimal content policy (attached, doesn't do anything but dump URLs coming through content policies to console). Which certainly means that NoScript, GreaseMonkey & Co. are affected as well.

Will try to narrow down the regression range with hourly builds though I already have a pretty good idea where this regressed,

Comment 1

[Wladimir Palant]

Regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1b3947ed93c6&tochange=29c228a4d7eb, so this is most certainly fallout from JavaScript compartments.

Comment 2

[Wladimir Palant]

http://flashbuilder.eu/flash-player-version.html is another website that is affected, fails to display version number of the installed Flash plugin. This page is simpler if somebody needs a testcase.

Comment 3

[Gary [:streetwolf52]]

Confirming....

Comment 4

[Andreas Gal :gal]

Can you see any error messages in the JS console?

Comment 5

[Wladimir Palant]

On flashbuilder.eu - no, nothing but a bunch of Analytics-related warnings.

Comment 6

[Benjamin Smedberg]

This is different from bug 594482, which is Java but much older?

Comment 7

[ToBo]

Here are other sample:

http://www.ardmediathek.de/ard/servlet/
http://www.zdf.de/ZDFmediathek/hauptnavigation/startseite/#/hauptnavigation/startseite

If i deactivate adblock on addon manager the sites work

Also the windowsmediaplayer as another player don't work on this site, if adblock activated...

Comment 8

[Yakove]

Same happens with Vkontakte videos like
http://vkontakte.ru/video5509074_151026486

Comment 9

[Wladimir Palant]

(In reply to comment #6)
> This is different from bug 594482, which is Java but much older?

Yes, definitely different. This bug is about a recent regression, caused by compartments landing, bug 594482 has been reported a while ago however.

Comment 10

[Benjamin Smedberg]

What I mean is: does any JS content policy trigger this bug, even the no-op one from that bug? If so, the two bugs are probably very much related and we have a smaller testcase to deal with.

Comment 11

[Wladimir Palant]

Yes, the two bugs have the same no-op content policy attached - and both are reproducible with it. And they probably both have something to do with wrappers created at the wrong time (as such bugs typically do). But I would be very surprised if the cause is the same in both cases.

Comment 12

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

This bug exist also on XP

Comment 13

[Gary King]

I cannot see the video in coment 8 nor the videos at:

http://www.cnet.com
http://www.nasa.gov

Comment 14

[Paul [pwd]]

I cannot see the install button on http://www.tweetdeck.com/desktop/

Comment 15

[Andreas Gal :gal]

#14: The "download now, its free" button? Do you have adblock installed?

Comment 16

[Paul [pwd]]

(In reply to comment #15)
> #14: The "download now, its free" button? Do you have adblock installed?

My bad, just checked with ABP disabled and it shows. However enabled with the page white-listed completely it still doesn't show. Not sure if that info helps this bug or not though.

Comment 17

[Gary [:streetwolf52]]

Just wanted to add that this bug also partially breaks the Add-on Stylish. As a result of all the 'breakage' I went back to cset 3c762301b719 and everything that wasn't working now does.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101013 Firefox/4.0b8pre - Build ID: 20101013173441

Comment 18

[d.a.]

I can confirm that the download button does not show up as long as ABP is enabled. As soon as it is disabled the button will show.

I think this problem appears on any site using swfobject to display Flash content. Simply creating a Flash object via JavaScript using createElement does not cause the bug to appear. So it must be something else in the swfobject code that makes this bug appear.

Source-code for swfobject can be downloaded here:
http://code.google.com/p/swfobject/downloads/list

Comment 19

[d.a.]

Attachment: Greasemonkey Script workaround

The problem occurs if the Flash movie is loaded through the data attribute of the object element. If it loads the Flash movie through the embed element it will show up.

I've created a Greasemonkey script which should work around the issue until a proper patch is available.

The script will only add an embed element if no such element is present in the object element. It will also append the flashvars attribute to the embed source url if it is present.

I think this will fix most broken Flash movies, but I'm not sure.

Comment 20

[Benjamin Smedberg]

bz, what does your blocker list look like?

Comment 21

[d.a.]

Attachment: Updated Greasemonkey Script

Some object tags hadn't been created after the 500ms delay so I upped it to 800ms, in addition I've made it run a second time 5 seconds after the first one is done, that should catch any late objects. 

I've tried using DOMSubtreeModified on some other scripts but I got quite a high CPU load. Any cleaner way to get it done?

Comment 22

[d.a.]

Attachment: Updated Greasemonkey Script, take two.

Some Flash objects did not load (such as YouTube embed), because the embed-element required the type-attribute to be present.

I also tried again with DOMSubtreeModified and now I can't see any increase in CPU usage.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> bz, what does your blocker list look like?

Nasty, brutish, and short.  ;)

I was meaning to look into this anyway.

Comment 24

[Jim Jeffery not reading bug-mail 1/2/11]

The sites listed in this post are also affected and do not show videos if ABP+ is installed.  The other posts in the thread have a GreaseMonkey hack to make things work: 

http://forums.mozillazine.org/viewtopic.php?p=10029773#p10029773

Comment 25

[Jo Hermans]

(In reply to comment #18)
> I can confirm that the download button does not show up as long as ABP is
> enabled. As soon as it is disabled the button will show.

In my case, I had to remove AdBlock Plus (version 1.2.2) completely. Disabling wasn't enough. Sigh....

Comment 26

[Gary King]

Until this bug land or is fixed, Minefield in rendered useless. Cannot see any flash content that is embedded. Most Major news outlets/site are no longer viewable.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #26)
> Until this bug land or is fixed, Minefield in rendered useless. Cannot see any
> flash content that is embedded. Most Major news outlets/site are no longer
> viewable.

First, please avoid evangelism in bugs. This comment adds no new information.
Second, disable or uninstall AdBlock Plus, and you should be fine.

Comment 28

[Peter Van der Beken [:peterv]]

It looks like the problem is two things. First of all we end up trying to get valueOf on the Xray for converting to string. Second we now call NewResolve more (we store the properties defined on the Xray on its holds), and the second call to NewResolve ends up calling nsObjectFrame::PrepareInstanceOwner which calls StopPluginInternal. More tomorrow.

Comment 29

[Giorgio Maone [:ma1]]

(In reply to comment #28)
>  First of all we end up trying to get
> valueOf on the Xray for converting to string.

This is likely an artifact of the dump() call in the minimal test content policy, which logs the node (context) parameter:

dump("shouldLoad: " + contentType + " " +
                          (contentLocation ? contentLocation.spec : "null") + " " +
                          (requestOrigin ? requestOrigin.spec : "null") + " " +
                          node + " " +
                          mimeTypeGuess + "\n");

I guess real world content policies don't log like this by default.

Comment 30

[Peter Van der Beken [:peterv]]

Yes, that's why I pointed out that there's two problems. Removing the dump and getting .ownerDocument results in the same problem.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: Fairly minimal testcase (linking to external swf) that shows the problem

Comment 32

[Boris Zbarsky [:bzbarsky]]

We end up with two separate plugin instance owners here.  The first is set up by nsObjectFrame::PrepareInstanceOwner in response to OnStartRequest.  The second is set up by the same in response to nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe.  The second is what ends up set up correctly, but the first is what we try to pump data into.

In fact, the second is set up while trying to set up the first:

#5  0x009b725c in nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe (wrapper=0x1d1e1380, obj=0x1f0887f8, _result=0xbfffa63c) at /Users/bzbarsky/mozilla/vanilla/mozilla/dom/base/nsDOMClassInfo.cpp:9385
#6  0x009bd6a4 in nsHTMLPluginObjElementSH::NewResolve (this=0x21a64b30, wrapper=0x1d1e1380, cx=0x61236b0, obj=0x1f0887f8, id={asBits = 106957312}, flags=1, objp=0xbfffa6dc, _retval=0xbfffa6e0) at /Users/bzbarsky/mozilla/vanilla/mozilla/dom/base/nsDOMClassInfo.cpp:9744
...
#29 0x005cf853 in nsContentPolicy::ShouldLoad (this=0x4d9e8b0, contentType=5, contentLocation=0x25803880, requestingLocation=0x1d1d2330, requestingContext=0x25803684, mimeType=@0xbfffc13c, extra=0x0, decision=0xbfffc158) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsContentPolicy.cpp:218
#30 0x00208d4e in NS_CheckContentLoadPolicy (contentType=5, contentLocation=0x25803880, originPrincipal=0x1d1d2050, context=0x25803684, mimeType=@0xbfffc13c, extra=0x0, decision=0xbfffc158, policyService=0x0, aSecMan=0x0) at nsContentPolicyUtils.h:223
#31 0x011bd7cc in nsPluginHost::InstantiateEmbeddedPlugin (this=0x4e240b0, aMimeType=0x216893a8 "application/x-shockwave-flash", aURL=0x25803880, aOwner=0x21644b20) at /Users/bzbarsky/mozilla/vanilla/mozilla/modules/plugin/base/src/nsPluginHost.cpp:1004
...
#32 0x011b04e2 in nsPluginStreamListenerPeer::OnStartRequest (this=0x1d1d0b30, request=0x1d1e1a44, aContext=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/modules/plugin/base/src/nsPluginStreamListenerPeer.cpp:588
#33 0x00690011 in nsObjectLoadingContent::OnStartRequest (this=0x25803620, aRequest=0x1d1e1a44, aContext=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsObjectLoadingContent.cpp:738

The fact that we have that second load to start with is bug 484992.

Comment 33

[Boris Zbarsky [:bzbarsky]]

If I change the content policy shouldLoad implementation by adding this line:

  dump(node.myExpando + "\n");

then I get exactly the same failure symptoms on 1.9.2 as I'm getting on trunk: we end up going through XPC_NW_NewResolve for "myExpando" and land in the classinfo code, etc.  But for some reason we do NOT hit XPC_NW_NewResolve for node.ownerDocument, though we _do_ hit XPC_NW_GetOrSetProperty.  I have no idea how we're managing that, yet.

Comment 34

[Boris Zbarsky [:bzbarsky]]

So at the moment, I think the obvious options are:

1) Fix bug 484992
2) Change the classinfo resolve hook here to not resolve things on wrappers
3) Make the resolve behavior match 1.9.2, however that worked.

Comment 35

[Peter Van der Beken [:peterv]]

> then I get exactly the same failure symptoms on 1.9.2 as I'm getting on trunk:
> we end up going through XPC_NW_NewResolve for "myExpando" and land in the
> classinfo code, etc.  But for some reason we do NOT hit XPC_NW_NewResolve for
> node.ownerDocument, though we _do_ hit XPC_NW_GetOrSetProperty.  I have no idea
> how we're managing that, yet.

On trunk with revision edb0506b0c23 (just before compartments) I see XPC_NW_NewResolve for ownerDocument, and we have the exact same issue (two plugin instance owners). The only "regression" is that if you just dump the node we used to not get into NewResolve. We should really fix the two plugin instance owners.

Comment 36

[Peter Van der Beken [:peterv]]

... but that's with http://flashbuilder.eu/flash-player-version.html, not with attachment 484426 [details].

Comment 37

[Boris Zbarsky [:bzbarsky]]

As in, you don't see the issue with the minimal testcase pre-compartments, but do see it on hat flashbuilder link?

fwiw, I did check that the other two urls in comment 0 are also using <object>, not <embed>, to embed flash.

Comment 38

[Peter Van der Beken [:peterv]]

(In reply to comment #37)
> As in, you don't see the issue with the minimal testcase pre-compartments, but
> do see it on hat flashbuilder link?

Right.

Comment 39

[Boris Zbarsky [:bzbarsky]]

That's ... quite odd.  ;)  The minimal testcase just has the DOM that flashbuilder generates for the <object>...

Comment 40

[Boris Zbarsky [:bzbarsky]]

Oh, I bet ownerDocument isn't hitting NewResolve for me on the minimal testcase is that the shouldLoad call for the _original_ data load resolved it already.  OK.  I'll try just fixing bug 484992.

Comment 41

[Boris Zbarsky [:bzbarsky]]

The patch I'm about to post in bug 484992 fixes all the testcases mentioned in this bug, when using the attached minimal policy, whether as-is or with the modifications I mentioned in comment 33.

Comment 42

[d.a.]

Attachment: GM workaround script, final try

For those who don't want to wait for the patch to get pushed I've updated the earlier GM script which should work on just about every Flash object. The previous script had some issues on some sites.

Comment 43

[Rob Madden]

(In reply to comment #42)
> Created attachment 484714 [details]
> GM workaround script, final try
> 
> For those who don't want to wait for the patch to get pushed I've updated the
> earlier GM script which should work on just about every Flash object. The
> previous script had some issues on some sites.

script works greart for greasmonkey.

Comment 44

[Rob Madden]

I have x86_64 latrest build of firefox and thisd bug is also in it. The greasmonkey workaound works for x86_64 version also. The bug is not just in x86.

Comment 46

[logos]

as mentioned here: https://bugzilla.mozilla.org/show_bug.cgi?id=606529#c3 I must disable both AB+ and NS to get a flash video running on a site where it wouldn't run otherwise. I insist: both must be disabled here.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Yes, yes.  They both use a JS-implemented content policy.  Now can we please stop adding more noise to this bug?  ;)

Comment 49

[Boris Zbarsky [:bzbarsky]]

Fixed by checkin for bug 484992.

Comment 50

[Rob Madden]

Latest tracemonkey does not have the fix for x86-64 and I am using windows 7 ultimate x64.

Comment 51

[Boris Zbarsky [:bzbarsky]]

What does comment 50 have to do with this bug?

Comment 52

[Rob Madden]

the bug is also in the 64 bit version.

Comment 53

[thewolf86]

The fix for this bug has landed on mozilla-central, not Tracemonkey. It will not be in Tracemonkey branch for at least a week - probably longer.

Is there a way to lock bugs to further comments? I would suggest it be used here if so. BTW, this fixed my Farmville/Mafia Wars.

Comment 54

[Matthew Turnbull [Bluefang]]

Just to clarify comment 50 and comment 52

Some people have gotten into the [bad] habit of using Win64 builds from latest-tracemonkey rather than latest-trunk, because latest-trunk doesn't reliably have nightly builds.

Comment 55

[Rob Madden]

Central don't have the x86-64 bit version if it has the fix because it was uploaded on the 20th not not the 22nd for x86-64 for windows. I don't use the 32 bit version I use the 64 bit version only.

Comment 56

[Boris Zbarsky [:bzbarsky]]

> Is there a way to lock bugs to further comments?

No.  Else I would have used it around comment 47.

Rob, see comment 53.  This is fixed on m-c; it'll be fixed in tracemonkey the next time there's a merge from m-c to tracemonkey.