abhixecutor Reporter
	Description • 12 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120814224555

Steps to reproduce:

Go to : http://www.teradata.com/business.aspx?id=16740 play the video on the site.



Actual results:

When the video is closed firefox continues to play the music/audio of the video in the background.



Expected results:

The video along with the audio should stop and video shouldn't don't be downloaded.
This is being observed in many other sites too

	Loic
	Comment 1 • 12 years ago

what's your Flash version?

	abhixecutor Reporter
	Comment 2 • 12 years ago

(In reply to Loic from comment #1)
> what's your Flash version?

Flash 10.2 r153

	Loic
	Comment 3 • 12 years ago

Thanks, I'm able to reproduce it with Flash 11.2.202.235 too.

It's a regression from Firefox 12.

mozregression range:
m-c
good=2012-01-31
bad=2012-02-01
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3f26b7bee352&tochange=e18c7bc2c28e

Suspected bug:
Josh Aas — Bug 90268: Change plugin instance ownership from layout to content. r=roc r=bsmedberg

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 4 • 12 years ago

Dupe of https://bugzilla.mozilla.org/show_bug.cgi?id=781482 or visa-versa ?  or just not related at all...

	Loic
	Comment 5 • 12 years ago

Quickly, I would say the current bug is similar to bug 776451.

	abhixecutor Reporter
	Comment 6 • 12 years ago

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #4)
> Dupe of https://bugzilla.mozilla.org/show_bug.cgi?id=781482 or visa-versa ? 
> or just not related at all...

it is similar to https://bugzilla.mozilla.org/show_bug.cgi?id=781482#c7 
but the problem described in the thread i believe is that even after closing the tab the video continues to play but in this case once the tab is closed the video stops playing

	Ludovic Hirlimann [:Usul]
	Comment 7 • 12 years ago

Is this a dup of 784070 ?

	Loic
	Comment 8 • 12 years ago

(In reply to Ludovic Hirlimann [:Usul] from comment #7)
> Is this a dup of 784070 ?

No, bug 781482 or bug 784070 imply zombie compartments with add-ons, it's not the case here. You can reproduce the bug with a fresh profile without add-ons.

	Loic
	Comment 9 • 12 years ago

Another example:
http://zizirincolisky.com/installations_en.html#confession_chair
Click on the image then click on the play button when you see the Flash video. Close the video (X button), the audio continues to play.

EDIT: and the regression is in FF13, not FF12.

	John Schoenick [:johns] Assignee
	Comment 11 • 12 years ago

This is caused by plugins losing their frame (e.g. becoming display:none) not properly stopping. Bug 783059 is needed as well to create tests for all these start/stop cases, I suspect we'll find more issues.

	John Schoenick [:johns] Assignee
	Comment 12 • 12 years ago

I think we should wait until instantiation tests are in before changing stuff
around like this, but this re-purposes InDocCheckEvent to also handle killing
plugins that lose frames.

	John Schoenick [:johns] Assignee
	Comment 13 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=61dd575c1fdf

	Alex Keybl [:akeybl]
	Comment 14 • 12 years ago

On further review, this is not a recent regression and hasn't been a major issue on SUMO since FF13 was on beta. No need to track for release, although we'd consider an uplift when ready.

	John Schoenick [:johns] Assignee
	Comment 15 • 12 years ago

Un bitrotted. Repurposes InDocCheckEvent to check for both in-active-document
and has-a-frame, properly stopping plugins that do not get a new frame.

	Josh Aas
	Comment 17 • 12 years ago

Comment on attachment 692035 [details] [diff] [review]
Kill plugins that become unrendered (e.g. display:none)

Review of attachment 692035 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsObjectLoadingContent.cpp
@@ +939,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (!aFrame) {
> +    // Lost our frame. If we arn't going to be getting a new frame, e.g. we've

"arn't" -> "aren't"

	John Schoenick [:johns] Assignee
	Comment 22 • 12 years ago

Comment on attachment 724508 [details] [diff] [review]
Kill plugins that become unrendered (e.g. display:none)

I reworked this a little - we now track pending instances of this event so we don't queue event A -> get a new frame -> lose that frame -> queue event B -> event A fires before it's reasonable to assume we will not be getting a new frame.

	John Schoenick [:johns] Assignee
	Comment 24 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=4f7b470af486

	Josh Aas
	Comment 25 • 12 years ago

Comment on attachment 724508 [details] [diff] [review]
Kill plugins that become unrendered (e.g. display:none)

Review of attachment 724508 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsObjectLoadingContent.cpp
@@ +677,5 @@
>  }
>  
>  nsObjectLoadingContent::nsObjectLoadingContent()
>    : mPendingInstantiateEvent(nullptr)
> +  , mPendingCheckPluginStopEvent(nullptr)

You don't need to initailize comptrs to null.

@@ +993,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (!aFrame) {
> +    // Lost our frame. If we arn't going to be getting a new frame, e.g. we've

s/arn't/aren't/

	John Schoenick [:johns] Assignee
	Comment 26 • 12 years ago

Fixed typo and removed useless comptr initializers

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 27 • 12 years ago

(In reply to John Schoenick [:johns] from comment #24)
> https://tbpl.mozilla.org/?tree=Try&rev=4f7b470af486

Just tested with the 'try-build', and it fixes the issue with the audio-run on in the test URL.

http://www.teradata.com/business.aspx?id=16740

	John Schoenick [:johns] Assignee
	Comment 28 • 12 years ago

Rebased onto bug 818785 to fix try failures

	John Schoenick [:johns] Assignee
	Comment 29 • 12 years ago

(In reply to John Schoenick [:johns] from comment #28)
> Rebased onto bug 818785 to fix try failures

On to bug 851378, rather

	John Schoenick [:johns] Assignee
	Comment 30 • 12 years ago

Looks good on try now, despite orange factor

https://tbpl.mozilla.org/?tree=Try&rev=dea28098a353

	John Schoenick [:johns] Assignee
	Comment 31 • 12 years ago

Rebased again, on top of bug 843671 + bug 851378

	John Schoenick [:johns] Assignee
	Comment 32 • 12 years ago

Comment on attachment 726948 [details] [diff] [review]
Kill plugins that become unrendered (e.g. display:none). r=josh

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d40b8591124

	John Schoenick [:johns] Assignee
	Comment 33 • 12 years ago

Comment on attachment 724509 [details] [diff] [review]
Cleanup some dead code from nsObjectLoadingContent

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e7d0ad184c
https://hg.mozilla.org/integration/mozilla-inbound/rev/261c8ee45444
https://hg.mozilla.org/integration/mozilla-inbound/rev/da937d761b89

	Ed Morley [:emorley]
	Comment 34 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/f4e7d0ad184c
https://hg.mozilla.org/mozilla-central/rev/261c8ee45444
https://hg.mozilla.org/mozilla-central/rev/da937d761b89
https://hg.mozilla.org/mozilla-central/rev/0d40b8591124

	John Schoenick [:johns] Assignee
	Comment 35 • 12 years ago

Per comment 14, do we want to consider this for aurora or esr17?

It has been broken since the ff13 timeframe, but is still a regression, and any fallout from this fix would probably not be found until at least the beta audience anyway, given the usual way plugin issues are found.

	John Schoenick [:johns] Assignee
	Comment 36 • 12 years ago

Err, I suppose I should just nom the patches rather than tracking

	John Schoenick [:johns] Assignee
	Comment 37 • 12 years ago

Comment on attachment 726948 [details] [diff] [review]
Kill plugins that become unrendered (e.g. display:none). r=josh

Note this requires uplifting bug 851378 as well, which is also required for uplifting bug 843671

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Probably bug 90268, landed in FF13

User impact if declined:
Some sites, such as CNN, rely on setting display:none to kill off unwanted plugins such as video. Without this patch those instances continue to play in the background until the page is closed.

Testing completed (on m-c, etc.):
on m-c, not likely to see any fallout until it reaches beta regardless, given the low testing exposure plugins get on >=aurora

Risk to taking this patch (and alternatives if risky):
Fairly low, plugins have always been finicky about ordering, but the tests landed with bug 783059 should make serious regressions less likely.

String or UUID changes made by this patch: 
None

If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Regression from FF13 timeframe with impact on significant sites

Fix Landed on Version:
22

	John Schoenick [:johns] Assignee
	Comment 38 • 12 years ago

Comment on attachment 726948 [details] [diff] [review]
Kill plugins that become unrendered (e.g. display:none). r=josh

I take it back, bug 851378 is undesirable for esr17 and has no other reason to be uplifted there.

	Benjamin Smedberg
	Comment 39 • 12 years ago

Unless we need this to also fix the plugin comment UI, I'm not sure taking this on Aurora is worth the risk.

	bhavana bajaj [:bajaj]
	Comment 40 • 12 years ago

John, based on comment# 39 & given this is longstanding regression from FF13 it seems safer for this to ride the trains instead of uplift here in the last week of aurora cycle.

Please let us know if I mis-read anything here and there is a high value with the uplift given the risk else I will go ahead and a - these . Thanks !

	John Schoenick [:johns] Assignee
	Comment 41 • 12 years ago

(In reply to bhavana bajaj [:bajaj] from comment #40)
> John, based on comment# 39 & given this is longstanding regression from FF13
> it seems safer for this to ride the trains instead of uplift here in the
> last week of aurora cycle.
> 
> Please let us know if I mis-read anything here and there is a high value
> with the uplift given the risk else I will go ahead and a - these . Thanks !

I agree -- we also have a probable regression in bug 854082