798194 - When the URL bar scrolls out of view, content that's moved in is not invalidated / repainted

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) Load http://tinyurl.com/9zftqoc
 (2) Quickly pan all the way to the bottom of the page

There's an unpainted white space just about the size of the URL bar that's left at the bottom.

The problem appears to be that when the content iframe is resized, the scroll offset is clamped to a value less than it was before being resized.  Somehow TabChild/apzc aren't renegotiating the displayport update that needs to happen, or gecko isn't invalidating the newly scrolled-into-view content.

We could fix this in the platform, but the current implementation of scroll-url-bar-out-of-view is fantastically expensive: it'll reflow and repaint the content area on every frame while it moves out of view.  I'd rather fix that, which will incidentally make this bug go away and get rid of some more scrolling jank.

Comment 2

[Ben Francis [:benfrancis]]

Hi Chris,

I implemented the scroll address bar off screen feature after our discussion in Sao Paulo and I remember you saying at the time that what we're trying to do is inherently very expensive. If there's a way the current implementation can be improved to increase performance I'd really value your input because it currently makes the whole browser feel very laggy.

Comment 3

[Joe Drew (not getting mail)]

Is this still going to be implemented in Gaia, or is the 'correct' fix (in Platform) necessary?

Comment 4

[Ben Francis [:benfrancis]]

I'm also not sure whether there's any more work that needs to be done here on the Gaia side. It's possible that the refactoring work done in bug 800445 may have changed this situation.

Comment 5

[Joe Drew (not getting mail)]

OK - Chris, do you know if this bug is still valid? I certainly can't reproduce it on an Otoro.

Comment 6

[Joe Drew (not getting mail)]

Hmm, the URL bar also doesn't scroll out of view. That's probably related! :)

Comment 7

[Joe Drew (not getting mail)]

Okay, and now I just reproduced it. However, I still don't know if this is a platform bug or a Gaia bug.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes, I can still reproduce.  Unpainted content is a gecko bug.

Comment 9

[Joe Drew (not getting mail)]

Anthony, can you start out by reproducing this and then look into it?

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I can easily reproduce the problem.

Comment 11

[Andreas Gal :gal]

Whats the status here? This is a basecamp blocker.

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

There seems to be a problem updating the visible region with remote layer trees. I'm currently investigating TabParent/TabChild.

Comment 13

[Alex Keybl [:akeybl]]

Marking for C2, given this meets the criteria of known P1/P2 bugs at the end of C1.

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Quick fix for scrolling

The scroll point wasn't being clamped when the composition bounds were updated. There was also a threading issue that was causing the content repaint requests to get reordered.

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Clamp scrolling bounds, fix reordering and remove duplicated painting

Comment 16

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

Comment on attachment 684570 [details] [diff] [review]
Clamp scrolling bounds, fix reordering and remove duplicated painting

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1162,5 @@
> +  // Limit scroll offset based on composition bounds
> +  mFrameMetrics.mScrollOffset.x = NS_MIN(mFrameMetrics.mScrollOffset.x,
> +      NS_MAX(0.0F, mFrameMetrics.mScrollableRect.width - mFrameMetrics.mCompositionBounds.width));
> +  mFrameMetrics.mScrollOffset.y = NS_MIN(mFrameMetrics.mScrollOffset.y,
> +      NS_MAX(0.0F, mFrameMetrics.mScrollableRect.height - mFrameMetrics.mCompositionBounds.height));

This doesn't seem right to me. For one thing, mScrollOffset and mCompositionBounds don't use the same scale; the former is in CSS pixels and the latter is in device pixels. See FrameMetrics.h.

::: layout/ipc/RenderFrameParent.cpp
@@ +494,5 @@
> +    // calls to this function need to be queued to avoid reordering.
> +    mUILoop->PostTask(
> +      FROM_HERE,
> +      NewRunnableMethod(this, &RemoteContentController::DoRequestContentRepaint,
> +                        aFrameMetrics));

I thought we'd decided to have this always be called on the UI thread and have APZC dispatch to the UI thread internally if necessary before calling this?

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> ::: layout/ipc/RenderFrameParent.cpp
> @@ +494,5 @@
> > +    // calls to this function need to be queued to avoid reordering.
> > +    mUILoop->PostTask(
> > +      FROM_HERE,
> > +      NewRunnableMethod(this, &RemoteContentController::DoRequestContentRepaint,
> > +                        aFrameMetrics));
> 
> I thought we'd decided to have this always be called on the UI thread and
> have APZC dispatch to the UI thread internally if necessary before calling
> this?

This approach is simpler. Changing the APZC to keep track of threads probably isn't worthwhile unless we want to remove locking.

Comment 18

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

We already discussed why we can't remove locking.

One problem with this approach is that it requires two trips through the UI thread event loop to do anything.

Comment 19

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

You could at least just fold DoRequestContentRepaint into RequestContentRepaint.

That still leaves us in a situation where we sometimes call browser->UpdateFrame with FrameMetrics that are stale (the APZC has more up-to-date metrics already).

Comment 20

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix paint reordering

Fix paint reordering problem. No longer deals with duplicated events.

Comment 21

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix resizing of composition bounds

Comment 22

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

Comment on attachment 685437 [details] [diff] [review]
Fix paint reordering

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

This is OK although I think we should still also do the extra work to have DoRequestContentRepaint fetch the most up-to-date metrics from the APZC.

Comment 23

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=7090625c68bd

Comment 24

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix paint reordering v2

Same patch with proper hg headers.

Comment 25

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix resizing of composition bounds v2

Same patch with proper hg headers.

Comment 26

[Mozilla RelEng Bot]

Try run for 7090625c68bd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7090625c68bd
Results (out of 16 total builds):
    success: 14
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7090625c68bd

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f28b307848c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a032b7e86bbf

Comment 28

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

Anthony, please get these patches landed on beta. You have blocking-basecamp+ which is enough for approval.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f28b307848c9
https://hg.mozilla.org/mozilla-central/rev/a032b7e86bbf

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c27a510368f5
https://hg.mozilla.org/releases/mozilla-aurora/rev/52a5a90c6f26

https://hg.mozilla.org/releases/mozilla-beta/rev/f4fb0d55ea72
https://hg.mozilla.org/releases/mozilla-beta/rev/7849105e9206

Comment 31

[Mozilla RelEng Bot]

Try run for 7090625c68bd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7090625c68bd
Results (out of 17 total builds):
    success: 14
    warnings: 2
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7090625c68bd