arni2033 Reporter
	Description • 8 years ago
str

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160121030208
STR:
0. Set mouse option "When I rotate mouse wheel" to "scroll by 1 page"
   That's my default install.
1. Open the following "data:" url or click URL in the form above. Wait until flash loads.
>   data:text/html,<iframe src="https://www.youtube.com/v/60og9gwKh1o" height="344" width="425"></iframe><style>body{height:10000px;}iframe{position:fixed;}
2. Rotate mouse wheel down once.
3. If you still don't see the bug, then repeat Step 2.

Result:       
 Flash plug-in disappears when I scroll the page in Step 2 and doesn't reappear.
 I'll attach a screencast if nobody can reproduce it

Expectations: 
 Flash should be visible after Step 3.

This was caused by bug 1193055. Regression range:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b7a4595a79011dc47a6ac00eee59969586cccbe9&tochange=6b38c8223cc67a9e5c9167e276287e7e4fa8e890

	Tracy Walker [:tracy]
	Comment 1 • 8 years ago

interesting.  I could reproduce this the other day on Win 7.  My build has since been updated to Nightly 47.0a1, Build ID 20160128030208. And I can no longer reproduce it.  When I have time later, I'll reinstall the older build in attempt to prove it's a WFM now or not. 

In the mean time, arni, can you check with the latest Nightly build?

	arni2033 Reporter
	Comment 2 • 8 years ago

Of course it's reproducible. 20160129030206

	Jim Mathies [:jimm]
	Comment 3 • 8 years ago

This only reproduces with apz.

	Tracy Walker [:tracy]
	Comment 4 • 8 years ago

(In reply to Jim Mathies [:jimm] from comment #3)
> This only reproduces with apz.

Yep, just realized I had turned apz off for some other testing and forgot to turn it back.

	Jim Mathies [:jimm]
	Comment 5 • 8 years ago

Arni, could you test to see if you still see this bug in nightly?

	arni2033 Reporter
	Comment 6 • 8 years ago

I can't reproduce this on:   Win7_64, Nightly 46, 32bit, ID 20160208030244
Apparently fixed in bug 1243413

	Jim Mathies [:jimm]
	Comment 7 • 8 years ago

Thanks!

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 8 • 8 years ago

Marking unaffected for 46 since e10s will not be enabled on 46 release.

	arni2033 Reporter
	Comment 9 • 8 years ago

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
>>>   My Info:   Win7_64, Nightly 50, 32bit, ID 20160715063552 (2016-07-15)
I can still reproduce this

	arni2033 Reporter
	Comment 10 • 8 years ago
str

Updated STR:
0. Set pref "plugins.rewrite_youtube_embeds" to "false"
1. Open URL
2. Rotate mouse wheel down once
3. Rotate mouse up down once

AR:  Flash not visible
ER:  Should be visible

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 11 • 8 years ago

I can reproduce this in Nightly using the STR in comment 10. Just a note though that in Nightly the flash content doesn't disappear, but I think it gets replaced with the screenshot and is non-interactive. The underlying bug is still present though.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 12 • 8 years ago

Stepping through the code it looks like the nsGfxScrollFrame::HasPluginFrames() function is returning false, because the plugin frame is in the nsSubDocumentFrame, which is a direct child of the parent document's ViewportFrame. Therefore the plugin frame is not a descendant of the scrolling nsGfxScrollFrame, and doesn't trip the HasPluginFrames() check.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 13 • 8 years ago

So this is fixable by removing the ancestor check at [1] and just setting the value to true unconditionally. That way plugins anywhere on the page will cause paint-skipping to be disabled, and we'll always send the layer updates on scroll. That code was originally added when the codepath was reused to also set the plugin visibility, but that stuff was removed in [2] and so I don't anticipate any other side-effects from doing this.

However looking around the code a bit more I also see that there's some new code at [3] which seems related, and setting gfx.e10s.hide-plugins-for-scroll to false (and restarting the browser) fixes the issue.

Bob, since you added that stuff, do you know what the long-term plan is with respect to the hide-plugins-for-scroll pref and if removing the ancestor check is an acceptable fix to this problem?

[1] http://searchfox.org/mozilla-central/rev/1112b7a5222b71a3b5b68bd531f50ded6bcbc770/layout/generic/nsGfxScrollFrame.cpp#2051
[2] https://hg.mozilla.org/mozilla-central/rev/b1c3f87e64f3#l12.68
[3] http://searchfox.org/mozilla-central/rev/1112b7a5222b71a3b5b68bd531f50ded6bcbc770/layout/generic/nsGfxScrollFrame.cpp#2771

	arni2033 Reporter
	Comment 14 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> Just a note though that in Nightly the flash content doesn't disappear, but I think it
> gets replaced with the screenshot and is non-interactive.
Flash content on Nightly disappears during scrolling (turns into _transparent_ area that doesn't respond to clicks), and doesn't reappear after scrolling is finished.

Why would you even say "replaced with the screenshot" ? If Firefox developers expect Flash to be replaced with something, then something is not according to the plan, because it's NOT replaced.

	Bob Owen (:bobowen)
	Comment 15 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> So this is fixable by removing the ancestor check at [1] and just setting
> the value to true unconditionally. That way plugins anywhere on the page
> will cause paint-skipping to be disabled, and we'll always send the layer
> updates on scroll.

Is this definitely plugins anywhere on the page and not just a less strict check for plugin frames within our scrolling frame?
If it is, then couldn't we just check rootPresContext->NeedToComputePluginGeometryUpdates() and remove HasPluginFrames altogether?

> Bob, since you added that stuff, do you know what the long-term plan is with
> respect to the hide-plugins-for-scroll pref and if removing the ancestor
> check is an acceptable fix to this problem?

When I was changing it to do the captures at the start of scroll, I realised that I could improve the plugin scrolling, so that maybe we don't need to hide at all.
It still isn't perfect though, so I think we need to check with product whether they think it is good enough.
jimm might know the current status on this?

I agree, that removing the ancestor check sounds safe, given that as I understand it, this is an optimisation that we will just do less often when plugins get caught in this check.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 8 years ago

So this does as Bob suggested in the previous comment, and just removes most of the machinery surrounding the ancestor check since if we're getting rid of the ancestor check we don't need to walk through each plugin.

Also now because HasPluginFrames() returns true if there's a plugin anywhere on the page, we should never enter the condition inside the gfxPrefs::HidePluginsForScroll branch, so that is effectively dead code and can be removed as well. The only concern I have there is we might have entered it if for non-windowed plugins (because of the various #ifdef/XRE_IsContentProcess checks in HasPluginFrames). If those are still relevant I can reinstate the gfxPrefs::HidePluginsForScroll block.

	Bob Owen (:bobowen)
	Comment 17 • 8 years ago

Comment on attachment 8776960 [details] [diff] [review]
Patch

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

If removing the ancestor check is equivalent to just checking for any registered plugins, this looks correct to me.

	Jim Mathies [:jimm]
	Comment 18 • 8 years ago

Comment on attachment 8776960 [details] [diff] [review]
Patch

This looks ok to me.

	Jim Mathies [:jimm]
	Comment 19 • 8 years ago

> When I was changing it to do the captures at the start of scroll, I realised
> that I could improve the plugin scrolling, so that maybe we don't need to
> hide at all.
> It still isn't perfect though, so I think we need to check with product
> whether they think it is good enough.
> jimm might know the current status on this?

Filed bug 1291441.

	Pulsebot
	Comment 20 • 8 years ago

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4303e24fceb
Don't do paint skipping if there are plugin frames anywhere on the page, instead of limiting it to the scrollframe's descendants. r=jimm

	Carsten Book [:Tomcat]
	Comment 21 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e4303e24fceb

	Gerry Chang [:gchang]
	Comment 22 • 8 years ago

Hi :kats,
Since this bug is a regression and also affects 49/50, are you also considering to uplift this patch to 49/50?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 23 • 8 years ago

Arni/Tracy, can you verify this is fixed on Nightly? If so I'll request uplift.

	Tracy Walker [:tracy]
	Comment 24 • 8 years ago

Following STR's of comment 0, this looks good on Win 7 with latest Nightly 51.0a1, 20160815030201

	Tracy Walker [:tracy]
	Comment 25 • 8 years ago

also fine with STR's of comment #10 (Set pref "plugins.rewrite_youtube_embeds" to "false") with latest Nightly.

	arni2033 Reporter
	Comment 26 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/e4303e24fceb - fixed
https://hg.mozilla.org/mozilla-central/rev/37e1f09cd54e - not fixed
Fix range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=37e1f09cd54e&tochange=e4303e24fceb

By the way, now at last plugin doesn't blink. Still blinks on Firefox 48 and 49.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 27 • 8 years ago

Comment on attachment 8776960 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1192910 most recently
[User impact if declined]: windowed plugins can disappear or blink while scrolling
[Describe test coverage new/current, TreeHerder]: tested locally and verified in nightly
[Risks and why]: relatively low risk. the code has churned a bit recently so we should make sure the patches still apply and work properly on aurora/beta
[String/UUID change made/needed]: none

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 28 • 8 years ago

Comment on attachment 8776960 [details] [diff] [review]
Patch

e10s and APZ related regression, verified on Nightly, Aurora50+, Beta49+

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/bb72ce163b52

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 8 years ago

Needs rebasing for Beta.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 31 • 8 years ago

I had to rebase around bug 1252877, which is a rather significant change. Try push with the rebased version (we should probably test it manually before landing) is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fea2d365c51. Leaving ni on me for now.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 32 • 8 years ago

The try push seems clean. However I'm having trouble testing it locally because even if I use the beta build that the try push was based on [1] I can't reproduce the original problem, I just get black inside the iframe instead of the youtube flash player. Is anybody else able to test the build at [2] to verify this bug is fixed in it?

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=0339b0c18498
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fea2d365c51&selectedJob=25817057

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 33 • 8 years ago
str

I ended up using these STR:
0. Set pref "plugins.rewrite_youtube_embeds" to "false"
1. Open URL
2. Right-click on the flash player and select "Settings..." to get the little settings dialog
3. Rotate mouse wheel down once
4. Rotate mouse up down once

The settings dialog blinks as you scroll, and stays hidden if the bug manifests. Unfortunately it looks like my rebased patch didn't fix the bug. I'm wondering if it's safer to just wontfix this for 49...

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 34 • 8 years ago

Ok, I did a local build and figured out the problem with my patch. The attached version works for me. Jim, do you mind giving this a once-over since it wasn't a trivial rebase? If you're good with this I'll uplift to beta.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 35 • 8 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/a8ff10892230

	Paul Silaghi, QA [:pauly]
	Comment 36 • 8 years ago

Doesn't look very good in 49b8: http://screencast.com/t/sYOFuqM7
Thoughts?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 37 • 8 years ago

The flickering during scroll is expected. This bug is about it getting stuck in the "hidden" state, and from your screencast that appears to not happen.

	arni2033 Reporter
	Comment 38 • 8 years ago

(In reply to Paul Silaghi, QA [:pauly] from comment #36)
> Doesn't look very good in 49b8: http://screencast.com/t/sYOFuqM7
> Thoughts?

Just in case, this is bug 1212813 and duplicates

	Paul Silaghi, QA [:pauly]
	Comment 39 • 8 years ago

Thank you.
Verified fixed FX 49b8, 50.0a2 (2016-09-01), 51.0a1 (2016-09-01).