445520 - flashblock broken on trunk

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[(not currently active) Ted Mielczarek]

I don't have a smaller testcase here, sorry, but installing flashblock 1.5.6 and loading the URL, then clicking on the placeholder under "Adobe Flash Player" is broken.

Works: 2008062703
Broken: 2008062804

Suspicious checkins in that range: (and there are a lot)
mpalmgren
Friday, June 27, 2008 10:05:08 AM	352ccf413e87	Remove the element from the id map when it's removed from the document. b=441994 r+sr=roc 
jst
Friday, June 27, 2008 3:39:47 PM	ca7ab6dd624d	Fixing bug 421833. Deal better with plugin frames going away while initializing a plugin. r+sr=[e-mail]
jst
Friday, June 27, 2008 3:28:20 PM	ac132709a917	First part of the fix for bug 435334. Move the JVM auth tools from the OJI code into the plugin code. The code in question is in no way dependent on OJI and will be completely removed once the NPAPI has support for what this code offers. r=[e-mail], sr=[e-mail]
smichaud
Friday, June 27, 2008 1:12:58 PM	bbf964160ffb	Crash when closing pop-under window without focusing it. b=433644 r=josh sr=roc 
jst
Friday, June 27, 2008 4:20:53 PM	4ee9ac6759cd	Landing fix for bug 319371. Fix how we call NPP_GetURLNotify() in cases where the network load fails. Patch by [e-mail], r+sr=[e-mail]
jst
Friday, June 27, 2008 3:55:02 PM	2645e2e39b3b	Fixing bug 438830. Prevent instantiation of a plugin if it's already been instantiated. r+sr=[e-mail] 


If I had to guess, I'd say bug 421833 or bug 438830 are pretty likely targets.

Comment 1

[(not currently active) Ted Mielczarek]

Simpler testcase, still requires flashblock installed. Interestingly, with this testcase if I click, then click "Back" and then "Forward", the flash loads.

Comment 2

[(not currently active) Ted Mielczarek]

Backing out the patch from bug 438830 fixes this.
(Specifically, `hg export 2645e2e39b3b | patch -p1 -R`)

Comment 3

[(not currently active) Ted Mielczarek]

Note that Flashblock isn't broken in all cases, just when embedding using SWFObject 1.5 or older, or a similar method. Of course, this means youtube is broken, which is a bummer.

Comment 4

[Philip Chee]

Any ideas, guys?

Comment 5

[(not currently active) Ted Mielczarek]

I chatted with jst about this. He's aware of it, just hasn't had time to look into it. 

Comment 6

[Wladimir Palant]

Attachment: Minimized testcase

Attaching minimized testcase. The important bits here are: the Flash tag is removed from document and then reinserted, and its src attribute is removed and then set again. Apparently, we already have a plugin instance when we are changing the src attribute which is why the plugin is reinitialized.

Comment 7

[Wladimir Palant]

Attachment: Minimized testcase

Attaching minimized testcase. The important bits here are: the Flash tag is removed from document and then reinserted, and its src attribute is removed and then set again. Apparently, we already have a plugin instance when we are changing the src attribute which is why the plugin isn't reinitialized.

Comment 8

[Johnny Stenback (:jst)]

So what happens here is that when you click the icon in the flash area to unblock the flash plugin script (flashblock) ends up touching the plugin element. When that happens we instantiate the plugin so that the script sees the DOM node with the plugins scriptable properties etc. Now at this time the src attribute of the plugin is about:blank (or it has no source attribute), but we know its type so we go ahead and instantiate a flash plugin at this point. Then immediately after that the script sets the src of the plugin and we go to attempt to re-instantiate the plugin. At this point, the plugin frame exists (as it did earlier too), but we have not yet done the first reflow, so nsObjectLoadingContent::TryInstantiate() returns early and we never tell the flash plugin to load the actual flash animation.

bz, biesi, the code in TryInstantiate() came in as part of bug 390385, but it really doesn't seem correct to me. If the src attribute of a plugin changes, we *must* re-instantiate the plugin no matter what, don't we? If the plugin frame has been instantiated etc, but no yet reflown, we still need to kill the plugin and reinstantiate with the right URI AFAICT. Removing the check to see if the frame is not yet had its first reflow fixes this problem here.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Hm, I forgot the case that the plugin might've been instantiated before the first reflow. I think we still want to not instantiate before the first reflow in the normal case and only do it if there's a plugin already, because instantiating before reflow means the plugin doesn't know the right size yet.

Comment 10

[Johnny Stenback (:jst)]

Attachment: Re-instantiate plugin in TryInstantiate() if the frame already has a plugin instance.

Biesi, do you think this is good enough? If not, the other way I could see this being fixable would be to tear down the plugin in the frame once we get here so that the plugin gets re-instantiated once the frame is reflown. Not sure if that would work out better or not.

Comment 11

[Samuel Sidler (old account; do not CC)]

Johnny, is this good enough for the 1.9 branch? Can we get this landed on trunk tonight?

Comment 12

[Johnny Stenback (:jst)]

I wasn't able to land this yesterday due to the state of the tree, but it's in now. This is fine material for the 1.9 branch, especially since it's required for being able to land bug 438830 on the branch.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Pardon me for asking, but with all the different browser repositories and 
branches now being used, it's not clear to me if this patch landed in a 
place where it will immediately affect FF 3.0.2 or SM nightly trunk builds.  
Will it?

Comment 14

[Samuel Sidler (old account; do not CC)]

Comment on attachment 332387 [details] [diff] [review]
Re-instantiate plugin in TryInstantiate() if the frame already has a plugin instance.

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 15

[(not currently active) Ted Mielczarek]

(In reply to comment #13)
> Pardon me for asking, but with all the different browser repositories and 
> branches now being used, it's not clear to me if this patch landed in a 
> place where it will immediately affect FF 3.0.2 or SM nightly trunk builds.  
> Will it?

The "trunk" right now is mozilla-central, which is building Firefox 3.1a2pre. The latest SeaMonkey nightlies should be building using that same code as well (although I don't know the version number they're using). This patch hasn't landed on the 1.9.0 branch (which will produce Firefox 3.0.2) yet, but bug 438830, which caused the problem, hasn't landed there either.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Thanks Ted, Very helpful.

Comment 17

[Johnny Stenback (:jst)]

Fix landed in CVS.

Comment 18

[(not currently active) Ted Mielczarek]

Verified in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/20080816032044 Minefield/3.1a2pre

Thanks!

Comment 19

[Philip Chee]

Ted, Re-Open based on Bug 450949 ?

Comment 20

[(not currently active) Ted Mielczarek]

I don't have the Shockwave plugin, so I can't confirm this.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

It's working great in SeaMonkey now, thanks!

Comment 22

[Philip Chee]

> Ted, Re-Open based on Bug 450949 ?
The crash occurs in the shockwave plugin and only on OSX. Leaving this as FIXED; confirmed on:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre)
Gecko/20080817033619 Minefield/3.1a2pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.2pre) Gecko/2008081705
GranParadiso/3.0.2pre

Comment 23

[Reed Loden [:reed]]

Backed out on CVS HEAD.

Comment 24

[Samuel Sidler (old account; do not CC)]

Comment on attachment 332387 [details] [diff] [review]
Re-instantiate plugin in TryInstantiate() if the frame already has a plugin instance.

Pushing out to 1.9.0.3.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 332387 [details] [diff] [review]
Re-instantiate plugin in TryInstantiate() if the frame already has a plugin instance.

Need the regression fixed before we can approve this

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 332387 [details] [diff] [review]
Re-instantiate plugin in TryInstantiate() if the frame already has a plugin instance.

bug 450949 seems to have been resolved by a newer Shockwave, let's try to go ahead with this.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 332387 [details] [diff] [review]
Re-instantiate plugin in TryInstantiate() if the frame already has a plugin instance.

Approved for 1.9.0.6, a=dveditz for release-drivers

Comment 28

[Johnny Stenback (:jst)]

Fix landed in CVS again.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

I'm running on Windows XP with Flashblock 1.5.6. Using Firefox 3.0.5 and the nightly 3.0.6 build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.6pre) Gecko/2009011606 GranParadiso/3.0.6pre), I am seeing no difference of behavior with the test cases here. Flashblock seems to be working normally in both instances.

Comment 30

[Samuel Sidler (old account; do not CC)]

Al, Flashblock would have broke if bug 438830 had landed alone. Since it works fine, that means that this patch indeed landed and that this bug is fixed on the branch.

Comment 31

[Al Billings [:abillings - ex-MoCo]]

Even though the behavior is the same in 1.9.0.5?

Comment 32

[Samuel Sidler (old account; do not CC)]

Yeah. Had we landed bug 438830 in 1.9.0.5, it would have broken Flashblock on the trunk. The other way we could have done this was a complete roll up patch in bug 438850 and your verification of this bug would be that Flashblock still worked because initially the patch there broke Flashblock. (Am I making sense? I feel like I'm only being semi-clear for some reason. *sigh*)