Closed Bug 1193055 Opened 9 years ago Closed 8 years ago

On first scroll, plugin lags before being hidden

Categories

(Core :: Panning and Zooming, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s m8+ ---
firefox46 --- fixed

People

(Reporter: nissan4321, Assigned: jimm)

References

()

Details

Attachments

(3 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150810030205

Steps to reproduce:

1) Enter http://www.gplanet.co.il/
2) Start smooth scrolling the page.
3) Observe that the center ad below the menu is jumpy while smooth scrolling, while all the other are fine.


Actual results:

When smooth scrolling the ad that below the menu is jumpy while others are fine.


Expected results:

All ads should scroll smoothly.
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86_64
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
Likely the same thing as bug 1166301. Might depend on the specific ad though, I don't see this behaviour on OS X. If the ad is Flash it might be bug 1157708 instead.
Depends on: 1166301
I can repro the problem on Linux. The jumpy ad is a Flash object, and so as expected, the patches in bug 1166301 don't help.
Depends on: 1157708
No longer depends on: 1166301
I re-tested this now that bug 1157708 is resolved (via bug 1137944). The plugin now becomes hidden during scrolling instead of lagging behind, except the first time the page is scrolled, when it lags for a bit before being hidden.

Jim, do you know if this is a known limitation of the fix in bug 1137944?
Depends on: 1137944
No longer depends on: 1157708
Flags: needinfo?(jmathies)
Summary: Jumpy ad while smooth scrolling the page → On first scroll, plugin lags before being hidden
(In reply to Botond Ballo [:botond] from comment #3)
> I re-tested this now that bug 1157708 is resolved (via bug 1137944). The
> plugin now becomes hidden during scrolling instead of lagging behind, except
> the first time the page is scrolled, when it lags for a bit before being
> hidden.
> 
> Jim, do you know if this is a known limitation of the fix in bug 1137944?

I can't reproduce the initial lag on first scroll here. AFAICT this is a dupe of bug 1214878.
Flags: needinfo?(jmathies)
Please see the attached video which demonstrates what I'm seeing. I have confirmed that my build includes the fix for bug 1214878.
It seems that you need to scroll fairly quickly after the plugin appears on page load, for the problem to manifest.
(In reply to Botond Ballo [:botond] from comment #6)
> It seems that you need to scroll fairly quickly after the plugin appears on
> page load, for the problem to manifest.

Interesting, looks like a real bug. Is that on linux? I'm not seeing this on Windows.
Yup, Linux. 

I'm not very familiar with the patches in bug 1137944, but could the difference have to do with the fact that on Windows, we "defer composition until we receive confirmation plugin window metrics have been updated"? Based on the symptoms it looks like we're "sneaking in" some composites before the plugin has had a chance to hide itself.
Assignee: nobody → jmathies
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: jmathies → nobody
Blocks: apz-desktop
This is related to the use of apz notifications to hide plugins, which currently are listened to in the content process. We need to move apz control of visibility of these windows to the browser process. I had a patch in bug 1137944 that did this, it would be pretty easy to bring this back.
Marking as blocking all-aboard-apz as per bug 1152049 comment 32.
Blocks: all-aboard-apz
No longer blocks: apz-desktop
Blocks: e10s-plugins
Correct me if I'm wrong - based on the discussion yesterday Jim will look into fixing this by listening for scroll start/stop in the parent process directly rather than the child process.
Assignee: nobody → jmathies
tracking-e10s: --- → m8+
No longer depends on: 1137944
Blocks: 1232184
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> Correct me if I'm wrong - based on the discussion yesterday Jim will look
> into fixing this by listening for scroll start/stop in the parent process
> directly rather than the child process.

Hey kats, are we going to run into problems like bug 1232184 with this? Here is the code I was thinking of resurrecting:

https://bugzilla.mozilla.org/attachment.cgi?id=8662474&action=diff

I'm concerned that a state change to WHEEL_SCROLL in AsyncPanZoomController::DispatchStateChangeNotification won't pick up on non-scrollable content. I'll put something together to test to see.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#3474
Flags: needinfo?(bugmail.mozilla)
In most cases, yes, APZ should know if content is scrollable or not. There's various CanScroll functions on the AsyncPanZoomController and Axis classes which should provide that information.

However, I don't know if special-casing WHEEL_SCROLL is the way to go here, because we want to do this for any async scroll, right? It would be better to use the existing TransformBegin/TransformEnd notifications for this, if that works. Then it should work for touch-scrolling when we enable that.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> In most cases, yes, APZ should know if content is scrollable or not. There's
> various CanScroll functions on the AsyncPanZoomController and Axis classes
> which should provide that information.
> 
> However, I don't know if special-casing WHEEL_SCROLL is the way to go here,
> because we want to do this for any async scroll, right? It would be better
> to use the existing TransformBegin/TransformEnd notifications for this, if
> that works. Then it should work for touch-scrolling when we enable that.

Thanks, I'll look at the CanScroll helpers to try and solve bug 1232184.

Right now this doesn't work though for a different reason, I'm seeing a lot intermittent state transitions from transform -> nothing -> transform during a single wheel scroll event. This throws the plugin window management off. Looking into that currently.
Attached patch wip (obsolete) — Splinter Review
this appears to work reliably. needs some cleanup.
(In reply to Jim Mathies [:jimm] from comment #14)
> Right now this doesn't work though for a different reason, I'm seeing a lot
> intermittent state transitions from transform -> nothing -> transform during
> a single wheel scroll event. This throws the plugin window management off.
> Looking into that currently.

If you can get backtraces for these transitions we might be able to suppress them using the StateChangeNotificationBlocker machinery. We haven't used this state change mechanism a lot so there's probably improvements we can do here and I wouldn't hesitate to do them if they help.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> (In reply to Jim Mathies [:jimm] from comment #14)
> > Right now this doesn't work though for a different reason, I'm seeing a lot
> > intermittent state transitions from transform -> nothing -> transform during
> > a single wheel scroll event. This throws the plugin window management off.
> > Looking into that currently.
> 
> If you can get backtraces for these transitions we might be able to suppress
> them using the StateChangeNotificationBlocker machinery. We haven't used
> this state change mechanism a lot so there's probably improvements we can do
> here and I wouldn't hesitate to do them if they help.

The state changes I was seeing only seemed to happen on pages that couldn't scroll. I filtered these using IsPannable(). Note though that the NotifyAPZStateChange still gets sent for these. I'll file a follow up on that.
This current wip isn't working out. IsPannable doesn't detect if a pan will happen in the direction the user is trying to pan, and the spurious state changes when content is at a scroll stop also throw things off. I need to find a point at which apz knows a scroll will actually occur.
I still feel the right approach here is to use the existing TransformBegin/TransformEnd notifications and fix up the spurious state changes. We rely on the TransformBegin/TransformEnd in other places to indicate when async transforms are happening, just like we want here, so if there are issues with that we should fix them up. If you can get a log with the APZC logging [1] enabled and/or backtraces for them that should tell me where the spurious state changes are coming from.

[1] https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/gfx/layers/apz/src/AsyncPanZoomController.cpp#73
The apz state seems to be flipping between wheel scroll and nothing repeatedly.

for one scroll of the wheel - 

oldstate=NOTHING  newstate=WHEEL_SCROLL
APZStateChange::TransformBegin
oldstate=WHEEL_SCROLL  newstate=NOTHING
APZStateChange::TransformEnd
oldstate=NOTHING  newstate=WHEEL_SCROLL
APZStateChange::TransformBegin
oldstate=WHEEL_SCROLL  newstate=NOTHING
APZStateChange::TransformEnd
oldstate=NOTHING  newstate=WHEEL_SCROLL
APZStateChange::TransformBegin
oldstate=WHEEL_SCROLL  newstate=NOTHING
APZStateChange::TransformEnd
..

test case: http://dagobah.net/flash/internet_trailer.swf

I'll look at the stacks, see if i can figure out why this happen. Definitely only occurs when content doesn't have scroll area available. Scrollable pages don't have this problem.
here's the stack for the wheel_scroll -> nothing state change during a scroll attempt:

 	xul.dll!mozilla::layers::AsyncPanZoomController::DispatchStateChangeNotification(mozilla::layers::AsyncPanZoomController::PanZoomState aOldState, mozilla::layers::AsyncPanZoomController::PanZoomState aNewState) Line 3548	C++
 	xul.dll!mozilla::layers::AsyncPanZoomController::SetState(mozilla::layers::AsyncPanZoomController::PanZoomState aNewState) Line 3471	C++
 	xul.dll!mozilla::layers::AsyncPanZoomController::CancelAnimation(mozilla::layers::CancelAnimationFlags aFlags) Line 2575	C++
 	xul.dll!mozilla::layers::OverscrollHandoffChain::CancelAnimations(mozilla::layers::CancelAnimationFlags aFlags) Line 93	C++
 	xul.dll!mozilla::layers::InputQueue::CancelAnimationsForNewBlock(mozilla::layers::CancelableBlockState * aBlock) Line 393	C++
>	xul.dll!mozilla::layers::InputQueue::ReceiveScrollWheelInput(const RefPtr<mozilla::layers::AsyncPanZoomController> & aTarget, bool aTargetConfirmed, const mozilla::ScrollWheelInput & aEvent, unsigned __int64 * aOutInputBlockId) Line 273	C++
 	xul.dll!mozilla::layers::InputQueue::ReceiveInputEvent(const RefPtr<mozilla::layers::AsyncPanZoomController> & aTarget, bool aTargetConfirmed, const mozilla::InputData & aEvent, unsigned __int64 * aOutInputBlockId) Line 45	C++
 	xul.dll!mozilla::layers::APZCTreeManager::ReceiveInputEvent(mozilla::InputData & aEvent, mozilla::layers::ScrollableLayerGuid * aOutTargetGuid, unsigned __int64 * aOutInputBlockId) Line 728	C++
 	xul.dll!mozilla::layers::APZCTreeManager::ProcessWheelEvent(mozilla::WidgetWheelEvent & aEvent, mozilla::layers::ScrollableLayerGuid * aOutTargetGuid, unsigned __int64 * aOutInputBlockId) Line 1070	C++
 	xul.dll!mozilla::layers::APZCTreeManager::ReceiveInputEvent(mozilla::WidgetInputEvent & aEvent, mozilla::layers::ScrollableLayerGuid * aOutTargetGuid, unsigned __int64 * aOutInputBlockId) Line 1121	C++
 	xul.dll!nsBaseWidget::DispatchAPZAwareEvent(mozilla::WidgetInputEvent * aEvent) Line 1155	C++
 	xul.dll!nsWindow::DispatchWheelEvent(mozilla::WidgetWheelEvent * aEvent) Line 3894	C++
btw, you don't need a plugin to see this, it reproduces on about:blank.
Here's the input queue log - 

INPQ: got a content response; block=0
INPQ: got a content response; block=0
INPQ: got a content response; block=0
INPQ: started new scroll wheel block 17A8A480 id 2 for target 19F26800
INPQ: not waiting for content response on block 17A8A480
INPQ: current block is ready with target 19F26800 preventdefault 0
oldstate=NOTHING  newstate=WHEEL_SCROLL
APZStateChange::TransformBegin
INPQ: got a target apzc; block=2 guid={ l=1, p=3, v=2 }
INPQ: processing input block 17A8A480; preventDefault 0 target 19F26800
INPQ: discarding processed scroll wheel block 17A8A480
INPQ: got a content response; block=2
INPQ: started new scroll wheel block 0C112ED0 id 3 for target 19F26800
oldstate=WHEEL_SCROLL  newstate=NOTHING
APZStateChange::TransformEnd
Ok, thanks. I'll take a look at this tomorrow when I'm back at home and have a wheel to test with. Flagging myself so I don't forget.
Flags: needinfo?(bugmail.mozilla)
Ok, so I looked into this. There's actually four scenarios when it comes to wheel scrolling:

1) "Smooth" wheel scroll, on a scrollable page
2) "Smooth" wheel scroll, on a non-scrollable page (e.g. about:blank)
3) "Instant" wheel scroll, on a scrollable page
4) "Instant" wheel scroll, on a non-scrollable page

The "smooth" vs "instant" refer to whether the general.smoothScroll pref is set (or the Smooth Scroll option is selected in the browser's Advanced preferences.

Right now we fire the TransformBegin/TransformEnd notifications for all four of these cases. I would argue that we should only fire it for case (1), and none of the others. It's obvious that we shouldn't fire it for cases 2 or 4, but case 3 is debatable. The way APZC handles that scenario is it goes into state WHEEL_SCROLL [1], adjusts the async transform, and then immediately goes back into state NOTHING [2]. Even if this does result in an actual scroll, there doesn't seem to be much point in hiding the plugin for the near-zero time that it takes to do this.

I have a patch that modifies some of this code so that we only fire the TransformBegin/TransformEnd in case (1). If you feel we should still fire it in case (3) I can take another look - it shouldn't be too hard to do.

[1] https://dxr.mozilla.org/mozilla-central/rev/1ec3a3ff68f2d1a54e6ed33e926c28fee286bdf1/gfx/layers/apz/src/AsyncPanZoomController.cpp#1762
[2] https://dxr.mozilla.org/mozilla-central/rev/1ec3a3ff68f2d1a54e6ed33e926c28fee286bdf1/gfx/layers/apz/src/AsyncPanZoomController.cpp#1772
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26)
> Created attachment 8705286 [details] [diff] [review]
> TransformBegin/TransformEnd only for smooth scroll wheel animations
> 
> Here's the patch mentioned above

That works perfectly. With my patches this bug and bug 1232184 are fixed.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25)
> Ok, so I looked into this. There's actually four scenarios when it comes to
> wheel scrolling:
> 
> 1) "Smooth" wheel scroll, on a scrollable page
> 2) "Smooth" wheel scroll, on a non-scrollable page (e.g. about:blank)
> 3) "Instant" wheel scroll, on a scrollable page
> 4) "Instant" wheel scroll, on a non-scrollable page
> 
> The "smooth" vs "instant" refer to whether the general.smoothScroll pref is
> set (or the Smooth Scroll option is selected in the browser's Advanced
> preferences.
> 
> Right now we fire the TransformBegin/TransformEnd notifications for all four
> of these cases. I would argue that we should only fire it for case (1), and
> none of the others. It's obvious that we shouldn't fire it for cases 2 or 4,
> but case 3 is debatable. The way APZC handles that scenario is it goes into
> state WHEEL_SCROLL [1], adjusts the async transform, and then immediately
> goes back into state NOTHING [2]. Even if this does result in an actual
> scroll, there doesn't seem to be much point in hiding the plugin for the
> near-zero time that it takes to do this.

Doesn't seem like this would be an issue, although the plugin window updates will round trip through content and probably lag. That would be an existing bug and not an issue here I think. Isn't smooth scrolling our default? I wonder how many people use instant. (I tried searching telemetry for a related probe but didn't find anything.)
(In reply to Jim Mathies [:jimm] from comment #27)
> > Here's the patch mentioned above
> 
> That works perfectly. With my patches this bug and bug 1232184 are fixed.

\o/ Whenever you're ready we can ask Botond to review my patch.

> Doesn't seem like this would be an issue, although the plugin window updates
> will round trip through content and probably lag. That would be an existing
> bug and not an issue here I think.

Agreed.

> Isn't smooth scrolling our default?

Yeah it seems to be, according to firefox.js.

> I
> wonder how many people use instant. (I tried searching telemetry for a
> related probe but didn't find anything.)

No idea. A bunch of tests do, I know that much :)
Attachment #8705286 - Flags: review?(botond)
Attachment #8701097 - Attachment is obsolete: true
Attachment #8705315 - Flags: review?(bugmail.mozilla)
removing some unnecessary code.
Attachment #8705315 - Attachment is obsolete: true
Attachment #8705315 - Flags: review?(bugmail.mozilla)
Attachment #8705316 - Flags: review?(bugmail.mozilla)
Comment on attachment 8705316 [details] [diff] [review]
Plugin window management visibility changes

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

This is partly a rubberstamp - the changes look straightforward enough that I'm okay r+'ing them but if you want a more detailed review somebody else (roc?) should probably do it.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +2179,5 @@
> +void
> +CompositorParent::ScheduleShowAllPluginWindowsAPZ()
> +{
> +  CancelableTask *pluginTask =
> +    NewRunnableMethod(this, &CompositorParent::ShowAllPluginWindowsAPZ);

nit: * before the space, not after

@@ +2202,5 @@
> +
> +void
> +CompositorParent::ScheduleHideAllPluginWindowsAPZ()
> +{
> +  CancelableTask *pluginTask =

ditto

::: gfx/layers/ipc/CompositorParent.h
@@ +392,5 @@
> +  void ScheduleHideAllPluginWindowsAPZ();
> +  void ShowAllPluginWindowsAPZ();
> +  void HideAllPluginWindowsAPZ();
> +  void ShowAllPluginWindows();
> +  void HideAllPluginWindows();

I don't understand the purpose of the "APZ" versions of these functions. Why not just call (Show|Hide)AllPluginWindows directly?
Attachment #8705316 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> Comment on attachment 8705316 [details] [diff] [review]
> Plugin window management visibility changes
> 
> Review of attachment 8705316 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is partly a rubberstamp - the changes look straightforward enough that
> I'm okay r+'ing them but if you want a more detailed review somebody else
> (roc?) should probably do it.
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +2179,5 @@
> > +void
> > +CompositorParent::ScheduleShowAllPluginWindowsAPZ()
> > +{
> > +  CancelableTask *pluginTask =
> > +    NewRunnableMethod(this, &CompositorParent::ShowAllPluginWindowsAPZ);
> 
> nit: * before the space, not after
> 
> @@ +2202,5 @@
> > +
> > +void
> > +CompositorParent::ScheduleHideAllPluginWindowsAPZ()
> > +{
> > +  CancelableTask *pluginTask =
> 
> ditto
> 
> ::: gfx/layers/ipc/CompositorParent.h
> @@ +392,5 @@
> > +  void ScheduleHideAllPluginWindowsAPZ();
> > +  void ShowAllPluginWindowsAPZ();
> > +  void HideAllPluginWindowsAPZ();
> > +  void ShowAllPluginWindows();
> > +  void HideAllPluginWindows();
> 
> I don't understand the purpose of the "APZ" versions of these functions. Why
> not just call (Show|Hide)AllPluginWindows directly?

Yeah you're right those can go. Originally I had apz specific code in them, but when I resurrected this patch I removed that. Will update.
Comment on attachment 8705286 [details] [diff] [review]
TransformBegin/TransformEnd only for smooth scroll wheel animations

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

I agree with objectives laid out in comment 25, but I'm not a huge fan of accomplishing them this way.

What do you think of doing the following instead:

  - Have just one state, WHEEL_SCROLL_ANIMATION.

  - In the SCROLLMODE_INSTANT case, don't set the state to anything.

  - In the SCROLLMODE_SMOOTH case, check to see if the animation
    wouldn't scroll anything, and if so, don't bother creating it
    at all. If we do create it, enter the WHEEL_SCROLL_ANIMATION state.
(In reply to Botond Ballo [:botond] from comment #33)
>   - In the SCROLLMODE_SMOOTH case, check to see if the animation
>     wouldn't scroll anything, and if so, don't bother creating it
>     at all. If we do create it, enter the WHEEL_SCROLL_ANIMATION state.

Can we easily detect this without duplicating a bunch of code? If so then I agree this is better, with s/WHEEL_SCROLL_ANIMATION/WHEEL_SCROLL/ (i.e. just use the existing state instead of needlessly renaming it).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #34)
> (In reply to Botond Ballo [:botond] from comment #33)
> >   - In the SCROLLMODE_SMOOTH case, check to see if the animation
> >     wouldn't scroll anything, and if so, don't bother creating it
> >     at all. If we do create it, enter the WHEEL_SCROLL_ANIMATION state.
> 
> Can we easily detect this without duplicating a bunch of code? If so then I
> agree this is better, with s/WHEEL_SCROLL_ANIMATION/WHEEL_SCROLL/ (i.e. just
> use the existing state instead of needlessly renaming it).

I think it's just

     (delta.x != 0) && mX.CanScroll(delta.x)
  || (delta.y != 0) && mY.CanScroll(delta.y)

CanScrollWithWheel(delta) does almost exactly this; we could adapt and use that if you prefer.
I'll take a look at why the MozMouseScrollFailed code at the top of the OnScrollWheel function isn't catching this already - it looks like it should be.
It was because GetCurrentWheelTransaction was returning null. So splitting up that if condition made this patch much smaller.
Attachment #8705286 - Attachment is obsolete: true
Attachment #8705286 - Flags: review?(botond)
Attachment #8705787 - Flags: review?(botond)
Comment on attachment 8705787 [details] [diff] [review]
TransformBegin/TransformEnd only for smooth scroll wheel animations

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

Thanks!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1752,5 @@
>    }
>  
> +  if (delta.x == 0 && delta.y == 0) {
> +    // Avoid spurious state changes and unnecessary work
> +    return nsEventStatus_eIgnore;

Other paths return eConsumeNoDefault - are you sure we want to return eIgnore here? It might not matter, just wanted to check.
Attachment #8705787 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #38)
> Other paths return eConsumeNoDefault - are you sure we want to return
> eIgnore here? It might not matter, just wanted to check.

I felt eIgnore was more appropriate, but the return value is never used so it doesn't matter at all.
I'm going to put both of these together for a try push and then push to inbound. Kats, if you have any changes you'd like to make to your work please post the latest.
Nope, I'm happy with the patch on the bug. Just update the r? to r= in the commit message when landing. Thanks!
shoot, I forgot to remove those extra methods. will push a followup.
Depends on: 1241550
Blocks: 1243413
No longer blocks: 1243413
Depends on: 1243413
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: