Closed Bug 1243413 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
e10s m9+ ---
firefox46 --- wontfix
firefox47 --- fixed

People

(Reporter: alice0775, Assigned: jimm)

References

(Blocks 1 open bug, )

Details

Attachments

(4 files, 3 obsolete files)

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.
Oops
> 3. Click [VIDEO] link on header bar.
3. Click [Videos] link on header bar.
Disabling e10s also disables apz. Could you test this with e10s on but apz off?
(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.
Flags: needinfo?(jmathies)
In the apz case, this looks like a corner case we didn't cover in bug 1193055.
Depends on: 1193055
Flags: needinfo?(jmathies)
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.
Blocks: e10s-plugins
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.
Flags: needinfo?(mstange)
Attached file test case
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.
Flags: needinfo?(mstange)
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.
Attached patch dom fix wip (obsolete) — Splinter Review
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.
You can use nsLayoutUtils::IsAncestorFrameCrossDoc.
Attached patch dom scroll fixSplinter Review
That worked.
Assignee: nobody → jmathies
Attachment #8713746 - Attachment is obsolete: true
Attached file 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.
Attachment #8713772 - Flags: feedback?(bugmail.mozilla)
This appears to be a dupe.
Blocks: 1241550
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.
Attached patch Fix for APZ case (obsolete) — Splinter Review
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)
Attached patch Fix for APZ case (obsolete) — Splinter Review
Same patch, now with -U8
Attachment #8713925 - Attachment is obsolete: true
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.
Flags: needinfo?(bugmail.mozilla)
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?
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
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 on attachment 8713765 [details] [diff] [review]
dom scroll fix

Lets get this part landed.
Attachment #8713765 - Flags: review?(mstange)
Note Markus is on PTO for a couple of weeks.
Attachment #8713765 - Flags: review?(mstange) → review?(roc)
Comment on attachment 8713765 [details] [diff] [review]
dom scroll fix

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

Test?
Attachment #8713765 - Flags: review?(roc) → review+
(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.
Keywords: leave-open
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
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
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?
Flags: needinfo?(bugmail.mozilla)
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
Attachment #8713765 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(bugmail.mozilla)
Attached patch apz fixSplinter Review
passes try and the plugin scroll tests.
Attachment #8713926 - Attachment is obsolete: true
Attachment #8715826 - Flags: review?(bugmail.mozilla)
Attachment #8715826 - Flags: review?(bugmail.mozilla) → review+
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
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
Attachment #8715826 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(jmathies)
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(andrei.vaida)
(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.
Flags: needinfo?(jmathies)
Flags: needinfo?(bugmail.mozilla)
(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.
Flags: needinfo?(andrei.vaida) → needinfo?(petruta.rasa)
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?
Flags: needinfo?(petruta.rasa) → needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
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.
Attachment #8715826 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8713765 [details] [diff] [review]
dom scroll fix

OK in m-c, please uplift to aurora.
Attachment #8713765 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marking affected for 46 so sheriffs will see the uplift request.
You need to log in before you can comment on or make changes to this bug.