1243413 - [e10s] Video(www.nicovideo.jp) disappears forever when scroll comments pane with mouse wheel

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Alice0775 White]

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/211a4c710fb6af2cad10102c4cabc7cb525998b8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0 ID:20160127030236

This problem does not happen if disabled e10s.

Reproducible: always

Steps To Reproduce:
1. Open http://www.nicovideo.jp/
2. Click [Log-in] on header bar. And then logged in.
3. Click [VIDEO] link on header bar.
4. Open any video.
   e.g. http://www.nicovideo.jp/watch/sm19668873
5. Click [>] play button.
6. Click comments pane at the right side of video.
7. Try to scroll up/down the comments with mouse wheel.
8. Repeat step 5 if any.


Actual Results:
Video disappears *forever*,  until I scroll the root view. 
Sound is no problem.

Expected Results:
Video should not disappear.

Comment 1

[Alice0775 White]

Oops
> 3. Click [VIDEO] link on header bar.
3. Click [Videos] link on header bar.

Comment 2

[u279076]

Disabling e10s also disables apz. Could you test this with e10s on but apz off?

Comment 3

[Alice0775 White]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #2)
> Disabling e10s also disables apz. Could you test this with e10s on but apz
> off?

In this case(e10s on but apz off), The behavior is slightly different.
Video disappears, but immediately after, it appears. I.e. Video flickers.

Comment 4

[Jim Mathies [:jimm]]

In the apz case, this looks like a corner case we didn't cover in bug 1193055.

Comment 5

[Jim Mathies [:jimm]]

In the dom scroll case, we hide the window while the comment scroll takes place. This might be fixable down in the gfx scroll frame code.

Comment 6

[Jim Mathies [:jimm]]

For dom scroll, we walk all the activity observers for the document [1], which appears to be too broad, we pick up every plugin for the entire page.

The nsGfxScroll frame has an mOuter, which has an mFrames list, I wonder if we could walk that instead. This way only children of the scroll frame would be targeted.

marcus, can you make any suggestions here?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2019

also cc'ing kats on the apz states issue.

Comment 7

[Jim Mathies [:jimm]]

Attachment: test case

Comment 8

[Markus Stange [:mstange]]

In NotifyPluginFramesCallback you could check whether the plugin frame is a descendant frame of the scroll frame's mOuter, and only hide/show the plugin if it is.
There's an edge case that this wouldn't handle (but which we already don't handle): If there's a plugin with multiple ancestor scroll frames, and those have overlapping scroll activity periods, then the plugin will be unhidden too soon. Not sure if you care about that.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

I don't have access to a windows machine today so I can't repro this right now but I can look at it over the weekend or on Monday. If you happen to know what states the APZ is going through or have a dump of the state changes, please post it and that will probably let me get to it sooner.

Comment 10

[Jim Mathies [:jimm]]

Attachment: dom fix wip

This works for plugins that aren't in sub documents. I was hoping the nsIFrame traversal would jump over sub document boundaries, but apparently it doesn't work that way. Need to dig a bit further.

Comment 11

[Markus Stange [:mstange]]

You can use nsLayoutUtils::IsAncestorFrameCrossDoc.

Comment 12

[Jim Mathies [:jimm]]

Attachment: dom scroll fix

That worked.

Comment 13

[Jim Mathies [:jimm]]

Attachment: apz log.txt

Here's the apz log. Using the test case posted here and scrolling using the scroll wheel over the scrollable div vs. the page. The plugin hides and never comes back.

Comment 14

[arni2033]

This is a regression from bug bug 1193055. Regression range:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b7a4595a79011dc47a6ac00eee59969586cccbe9&tochange=6b38c8223cc67a9e5c9167e276287e7e4fa8e890
See also bug 1241550

Comment 15

[Jim Mathies [:jimm]]

This appears to be a dupe.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Thanks for the log. It doesn't show any obvious problems unfortunately. I'll repro and investigate this weekend or on Monday. Leaving flag on me for now.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fix for APZ case

I don't think this is an APZ state bug. The states and messages from APZ seem to be fine. I debugged a bit more and it looks like after mDeferPluginWindows is set back to true, the next composite goes through the "no change" path of the plugin window code, and so the widget never knows to show the plugin windows again. I'm attach a WIP patch that fixed the problem for me, feel free to steal/rework the patch into something landable.

(This fixes the problem for me on the nicovideo.jp site and arni's iframe testcase, the "test case" attachment on this bug didn't load the plugin for me)

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fix for APZ case

Same patch, now with -U8

Comment 19

[Jim Mathies [:jimm]]

I tested again with apz enabled in mc nightly. When I scroll the scrollable div below the plugin in the test case posted here, the plugin window hides. This is due to the apz state changing from NOTHING to WHEEL_SCROLL. I also checked a local build with my dom scroll fix, and see the same thing.

I don't know how the apz system is managed currently, do we have individual AsyncPanZoomControllers per scrollable elements or is there one AsyncPanZoomController controlling everything for a document? Maybe there's no way to avoid this for the scrollable div case.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

There's individual AsyncPanZoomController instances per scrollable element. And yes, the apz state will change from NOTHING to WHEEL_SCROLL, but it should change back from WHEEL_SCROLL to NOTHING once the scrolling stops, and that should make the plugin window show again. I wasn't able to reproduce any scenario where the APZC permanently stays in the WHEEL_SCROLL state.

Can you try reproducing on the same test case with the patch I attached?

Comment 21

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #20)
> There's individual AsyncPanZoomController instances per scrollable element.
> And yes, the apz state will change from NOTHING to WHEEL_SCROLL, but it
> should change back from WHEEL_SCROLL to NOTHING once the scrolling stops,
> and that should make the plugin window show again. I wasn't able to
> reproduce any scenario where the APZC permanently stays in the WHEEL_SCROLL
> state.

Yeah I've reproduced that, there's a lack of layer updates coming over in this case. Your patch would fix this.

However the bug we're trying to fix here is that the plugin window hides. Can you think of any way we can filter that out in cases like this?

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Ah, I thought we were trying to fix the plugin window hiding *permanently*, not the plugin window hiding *at all*. The way to fix the latter would be to send over information about the nearest scrollable ancestor of the plugin to the compositor, and then in the compositor only hide the plugin window if that ancestor (or one of it's ancestors) is the one scrolling.

Comment 23

[Jim Mathies [:jimm]]

Comment on attachment 8713765 [details] [diff] [review]
dom scroll fix

Lets get this part landed.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Note Markus is on PTO for a couple of weeks.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8713765 [details] [diff] [review]
dom scroll fix

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

Test?

Comment 26

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> Comment on attachment 8713765 [details] [diff] [review]
> dom scroll fix
> 
> Review of attachment 8713765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Test?

Working on that. browser_pluginscroll is currently disabled so I'm trying to sort that out in bug 1213631.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c537facdae50

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> Ah, I thought we were trying to fix the plugin window hiding *permanently*,
> not the plugin window hiding *at all*. The way to fix the latter would be to
> send over information about the nearest scrollable ancestor of the plugin to
> the compositor, and then in the compositor only hide the plugin window if
> that ancestor (or one of it's ancestors) is the one scrolling.

I'm not sure how to approach this, can you provide any pointers on how this might be done?

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Yeah, sorry for the vague response. I'm not fully clear on the specifics of how plugins are handled in layout but I can think of two approaches to do this:

1) When collecting the plugin information to send over the compositor (I assume in nsRootPresContext::CollectPluginGeometryUpdates somewhere), we should also send over the scrollable ancestor for each plugin. So that means, for each plugin, do something like [1] to find the closest ancestor with an APZC, and send over the guid corresponding to that ancestor. The code for this is going to be tricky/brittle though because the conditions that cause a scrollable frame to get layerized for APZC-scrolling are complex and subject to change. Anyway, assuming we can get this done, then the compositor will know which APZCs "contain" the plugin, and then when those APZCs (and only those APZCs) scroll, we hide the plugins. The specific mechanism for this would be that when APZC calls ScheduleHideAllPluginWindows on the CompositorParent, it passes along its own guid. The CompositorParent code would need to take the guid from the plugin, and see if it matches. The CompositorParent would *also* need to walk up the ancestor chain from the plugin's guid to see if any of *those* guids match the APZC guid. So overall this approach is pretty complex.

2) This second approach should be simpler but I don't know about the layout side things enough to know if it's doable. In this approach, we assign each plugin a unique ID on the layout side. Then, when building the layer three, we annotate layers with the IDs of the plugins they contain. I'm assuming here that plugins are treated like regular frames for the most part, and the nsDisplayPlugin item gets assigned to a layer - that plugin's ID would then get added to the list of IDs on that layer. On the compositor side, when an APZC scrolls, we can find the layer that APZC corresponds to, and walk the layer subtree rooted at that point. We collect all the plugin IDs in that subtree, and hide all of those plugins. There's existing data structures in the APZ code (the hit-testing tree) that we can abuse a little bit to assist us with this if needed, but in general I think this approach is more straightforward than the previous one, if we can pull it off.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp?mark=679-697&rev=a49bae84297c#679

Comment 31

[Jim Mathies [:jimm]]

This sounds like a lot of work for a feature we plan to severely limit very soon through the switch to windowless flash. Do you think the hidden window portion of this bug should block apz or would a work around like the one you posted be sufficient to consider this fixed for apz rollout?

Comment 32

[Jim Mathies [:jimm]]

Comment on attachment 8713765 [details] [diff] [review]
dom scroll fix

Approval Request Comment
[Feature/regressing bug #]:
e10s related bug with windowed plugins and dom scroll
[User impact if declined]:
odd behavior on some sites. not critical.
[Describe test coverage new/current, TreeHerder]:
on mc, my own testing.
[Risks and why]: 
low to none. small well isolated change.
[String/UUID change made/needed]:
none

Comment 33

[Kartikaya Gupta (email:[e-mail])]

I think fixing the "forever" part of this bug with a patch like the one I posted should be sufficient. If you can get the screenshot thing working then it will fix the rest of this problem as well.

I agree that the complexity required to stop plugin hiding while scrolling something else on the page is probably not worth it, since the plugin hiding is going to happen when scrolling the page regardless.

Comment 34

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f174907c8a22

Comment 35

[Jim Mathies [:jimm]]

Attachment: apz fix

passes try and the plugin scroll tests.

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/556b33a57f0f

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/556b33a57f0f

Comment 38

[Jim Mathies [:jimm]]

Comment on attachment 8715826 [details] [diff] [review]
apz fix

Approval Request Comment
[Feature/regressing bug #]:
apz bug with plugin windows
[User impact if declined]:
buggy scroll behavior with plugins on the page
[Describe test coverage new/current, TreeHerder]:
just landed on mc 2/5
[Risks and why]: 
medium, this patch should bake on mc for a few days.
[String/UUID change made/needed]:
none

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei can your team verify this on m-c? Thanks!
Steps to reproduce are in comment 0 and comment 1. 

Jimm, kats: do we need to test this with APZ enabled and also with it disabled?

Comment 40

[Jim Mathies [:jimm]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #39)
> Andrei can your team verify this on m-c? Thanks!
> Steps to reproduce are in comment 0 and comment 1. 
> 
> Jimm, kats: do we need to test this with APZ enabled and also with it
> disabled?

Yes, two fixes landed. A work around for apz and a dom scroll fix. In the apz case the plugin will hide and reshow, in the dom case it should remain visible.

Comment 41

[Andrei Vaida [:avaida]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #39)
> Andrei can your team verify this on m-c? Thanks!
> Steps to reproduce are in comment 0 and comment 1. 

We're looking into it as we speak. We'll follow up with test results as soon as possible.

Comment 42

[Petruta Horea [:phorea], Desktop QA]

Reproduced the initial issue on Nightly 2016-01-27 on Win 7 64-bit.

Using latest Nightly 47.0a1 2016-02-09, the behavior improved, but there are still issues:
 APZ on: -scrolling outside the video area, makes the video disappear or be replaced by a black rectangle
         -scrolling the comments area makes the video disappear
 APZ off: -the video turns black when scrolling outside the video area
           -nothing happens when scrolling the comments

Link to a gif showing the remaining problems on latest Nightly (too big to attach in bugzilla): https://drive.google.com/file/d/0B7jBNZBM3mz-ZW1xZllWUTA1anM/view?usp=sharing

Is this the expected behavior as discussed in comments 31 - 33?

Comment 43

[Jim Mathies [:jimm]]

(In reply to Petruta Rasa [QA] [:petruta] from comment #42)
> Reproduced the initial issue on Nightly 2016-01-27 on Win 7 64-bit.
> 
> Using latest Nightly 47.0a1 2016-02-09, the behavior improved, but there are
> still issues:
>  APZ on: -scrolling outside the video area, makes the video disappear or be
> replaced by a black rectangle
>          -scrolling the comments area makes the video disappear
>  APZ off: -the video turns black when scrolling outside the video area
>            -nothing happens when scrolling the comments
> 
> Link to a gif showing the remaining problems on latest Nightly (too big to
> attach in bugzilla):
> https://drive.google.com/file/d/0B7jBNZBM3mz-ZW1xZllWUTA1anM/view?usp=sharing
> 
> Is this the expected behavior as discussed in comments 31 - 33?

yes, this represents the current state of things.

Comment 44

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8715826 [details] [diff] [review]
apz fix

Sounds like this still has issues, but improves the scrolling behavior. 
OK to uplift to aurora since we are aiming this feature to release with 46.

Comment 45

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8713765 [details] [diff] [review]
dom scroll fix

OK in m-c, please uplift to aurora.

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking affected for 46 so sheriffs will see the uplift request.