890897 - Defect - Overlay scrollbars trigger continual repaints on metro start page and irccloud.com

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jim Mathies [:jimm]]

Noticed this on the metro start page. Basically, the final fade out callback changes opacity on the scrollbars, which triggers a replow, which in turn triggers a call to ScrollbarActivity::ActivityOccurred() via nsGfxScrollFrameInner::CurPosAttributeChanged(), which starts the fade again. 

If you put a debug print statement in the refresh driver, you can see the behavior. Not sure if this reproduces on osx, but it does in metrofx on Win8.

Comment 1

[Jim Mathies [:jimm]]

Attachment: fix

Comment 2

[Stephen A Pohl [:spohl]]

This seems to match what is being observed in bug 870917. Would you happen to know why (and how) the current position changes when the fade out completes? That doesn't seem right to me.

Comment 3

[Jim Mathies [:jimm]]

Here's the stack. Looks like we always call CurPosAttributeChanged from within nsGfxScrollFrameInner::ReflowFinished.

xul.dll!mozilla::layout::ScrollbarActivity::ActivityOccurred() Line 54	C++
xul.dll!nsGfxScrollFrameInner::CurPosAttributeChanged(nsIContent * aContent=0x0a5563a8) Line 2948	C++
xul.dll!nsGfxScrollFrameInner::ReflowFinished() Line 3604	C++
xul.dll!PresShell::HandlePostedReflowCallbacks(bool aInterruptible=true) Line 3703	C++
xul.dll!PresShell::DidDoReflow(bool aInterruptible=true, bool aWasInterrupted=false) Line 7667	C++
xul.dll!PresShell::ProcessReflowCommands(bool aInterruptible=false) Line 8002	C++
xul.dll!PresShell::FlushPendingNotifications(mozilla::ChangesToFlush aFlush={...}) Line 3897	C++
xul.dll!nsRefreshDriver::Tick(__int64 aNowEpoch=1373290380164175, mozilla::TimeStamp aNowTime={...}) Line 1185	C++
xul.dll!mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver * driver=0x05624e88, __int64 jsnow=1373290380164175, mozilla::TimeStamp now={...}) Line 172	C++
xul.dll!mozilla::RefreshDriverTimer::Tick() Line 163	C++
xul.dll!nsTimerImpl::Fire() Line 543	C++
xul.dll!nsTimerEvent::Run() Line 629	C++

Comment 4

[Stephen A Pohl [:spohl]]

So nsGfxScrollFrameInner::SetCoordAttribute (called via nsGfxScrollFrameInner::FinishReflowForScrollbar in nsGfxScrollFrameInner::ReflowFinished) checks whether or not the attribute actually changed, but that doesn't seem to propagate out and CurPosAttributeChanged is called regardless.

This raises two questions:
1. Does the "current position" attribute actually change?
2. Why does this work in most cases, but not in the situation described here or in bug 870917?

One thing to point out is that at the end of the fade out we set the scrollbar visibility to 'hidden' in nativescrollbars.css:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/nativescrollbars.css#26
I wonder if that has anything to do with it.

Comment 5

[Jim Mathies [:jimm]]

If we keep this, I need to replace the ActivityOccurred call in HandleEvent to call ActivityStarted/ActivityStopped directly. In the real world missing a mousemove input event would never happen but it could in tests.

Comment 6

[Markus Stange [:mstange]]

(In reply to Stephen Pohl [:spohl] from comment #4)
> This raises two questions:
> 1. Does the "current position" attribute actually change?
> 2. Why does this work in most cases, but not in the situation described here
> or in bug 870917?

I think we need to answer these questions before we take a patch. I don't know the answers, unfortunately.

Comment 7

[Jim Mathies [:jimm]]

This causes a .5 - 1% cpu drain while sitting idle on the startpage on light weight tablets, so it blocks v1 release.

Comment 8

[Stephen A Pohl [:spohl]]

Adding lion-scrollbars+ keyword for tracking and to make it appear on our overlay scrollbar triage page:
https://wiki.mozilla.org/Lion_Scrollbars/Triage

Comment 9

[Marco Mucci [:MarcoM]]

Removing based on Jim's comment, "spohl mentioned he was looking at it in the parent bug."

Comment 10

[Stephen A Pohl [:spohl]]

Attachment: Patch

This fixes the issue on irccloud.com. However, I have a feeling that this patch might not fix the problem reported here. Jim, would you mind applying this patch and letting me know what's happening? Thanks!

Comment 11

[Jim Mathies [:jimm]]

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

Fixes the refresh problem for me.

Comment 12

[Stephen A Pohl [:spohl]]

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

Thanks Jim!

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Jim, could you try today's mozilla-central nightly (which contains Stephen's patch for bug 870917)?  I suspect it might fix this bug, all by itself.

Comment 14

[Jim Mathies [:jimm]]

(In reply to Steven Michaud from comment #13)
> Jim, could you try today's mozilla-central nightly (which contains Stephen's
> patch for bug 870917)?  I suspect it might fix this bug, all by itself.

I did this morning, it didn't.

Comment 15

[Stephen A Pohl [:spohl]]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02f261f13de5

Comment 16

[Stephen A Pohl [:spohl]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 636564
User impact if declined: Scrollbars can flicker continuously on some websites when we refuse to scroll because we're already in an acceptable place.
Testing completed (on m-c, etc.): Extensive testing locally.
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 17

[Stephen A Pohl [:spohl]]

This bug solves an aspect that was first reported in bug 870917, which was tracking for beta. I think we'd want this in FF23 as well.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/02f261f13de5

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/271d4c5b370e
https://hg.mozilla.org/releases/mozilla-beta/rev/d5c2206ce4e4

Comment 20

[Paul Silaghi, QA [:pauly]]

FF metro isn't available only in nightly 25 ?

Comment 21

[Paul Silaghi, QA [:pauly]]

I don't see any differences on metro builds from 2013-07-08 and 2013-07-31, but
(In reply to Jim Mathies [:jimm] from comment #0)
> If you put a debug print statement in the refresh driver, you can see the
> behavior. Not sure if this reproduces on osx, but it does in metrofx on Win8.
I don't know how to do this, so I'm going to need further instructions in order to verify this bug.

Comment 22

[Stephen A Pohl [:spohl]]

(In reply to Paul Silaghi [QA] from comment #21)
> I don't see any differences on metro builds from 2013-07-08 and 2013-07-31,

Jim, do you have any good advice for Paul here how to reproduce the issue on metro?

Comment 23

[Jim Mathies [:jimm]]

(In reply to Stephen Pohl [:spohl] from comment #22)
> (In reply to Paul Silaghi [QA] from comment #21)
> > I don't see any differences on metro builds from 2013-07-08 and 2013-07-31,
> 
> Jim, do you have any good advice for Paul here how to reproduce the issue on
> metro?

In debug builds you could turn on nspr nsRefreshDriver:1 logging.

I just checked in the debugger and can verify this is fixed for metrofx tip.

Comment 24

[Paul Silaghi, QA [:pauly]]

Thanks Stephen, Jim.

Comment 25

[u279076]

Assuming verified fixed based on comment 23.