799401 - URL bar scrolling out of view while a fling is in progress causes window to flicker

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P2)

Description

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

STR
 (1) Load nytimes.com in browser
 (2) Zoom all the way so that all content is fit in the window, leaving the URL bar in view
 (3) Give the page a good solid fling down

If the fling animation is still continuing when the URL bar moves off the screen, there's a rather severe entire-screen flash.

I'm stumbling across this in bug 795657, but it's a pre-existing condition.  Bug 798194 might make it obsolete.

Comment 2

[Jason Smith [:jsmith]]

Actually, this is better described bug. Let's use this as the real bug and I'll dup bug 797264 on this.

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

It doesn't always cause the content to flicker, though I have seen this:

Otoro phone, build 2012-10-09 us
Taken from default.xml in b2g-distro: 
* "platform_build" revision= 0d6d050bc37d3167cc82a1885fd7660456bb0f4e
* "gaia" revision= 5f7dbe06490e6b52a356e033f52fb31c0fa0e2d0 
* "releases-mozilla-central" revision= 1f41d3603a035d64c3b1d7c16dba5fd1ce322957
* "gonk-misc" revision= bac8185ab931c9bc868e9bcb9b636620b8032e5e

Comment 5

[JP Rosevear [:jpr]]

Jet, Joe, we need to get this one assigned out.

Comment 6

[Joe Drew (not getting mail)]

Since Anthony's looking into bug 798194, I'll let him weigh in on this after he's investigated that one.

Comment 7

[Alex Keybl [:akeybl]]

Milestoning for C2 (deadline of 12/10), as this meets the criteria of "known P2 bugs found before or during C1".

Comment 8

[Andreas Gal :gal]

What is the status here? No progress in 20 days for a basecamp blocker is unacceptable.

Comment 9

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

Not sure how to r(In reply to Andreas Gal :gal from comment #8)
> What is the status here? No progress in 20 days for a basecamp blocker is
> unacceptable.

I'm not sure how to respond to that.

Comment 10

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

Looks to me like the repainting isn't keeping up with the scrolling. There may also be an extra repaint occurring during the resize.

Comment 11

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

Attachment: Skip obsolete paint requests

I had trouble reproducing a flash but this patch makes the nytimes site perform a lot better. About 2/3rds of paint requests were out of date before even starting to paint. Skipping obsolete paint requests avoids a backlog of stale requests.

Would like some feedback on class names and file locations and the general concept.

Comment 12

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

You might want to try reverting http://hg.mozilla.org/mozilla-central/rev/9698936945c6 .

Comment 13

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

Comment on attachment 685964 [details] [diff] [review]
Skip obsolete paint requests

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

Looks good to me but this is more cjones' territory I think

Comment 14

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

Attachment: Bug 799401 - Throttle APZC paint requests; r=cjones

Comment 15

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

Comment on attachment 686366 [details] [diff] [review]
Bug 799401 - Throttle APZC paint requests; r=cjones

This would have been easier to review as a revert+rebase of
http://hg.mozilla.org/mozilla-central/rev/9698936945c6 optionally
followed by a refactoring patch, since the original code was well
understood.  But this is fine.

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp

>-  mGeckoContentController->RequestContentRepaint(mFrameMetrics);
>+  mPaintThrottler.PostTask(
>+    FROM_HERE,
>+    NewRunnableMethod(mGeckoContentController.get(),
>+                      &GeckoContentController::RequestContentRepaint,
>+                      mFrameMetrics));

The task created here doesn't hold a strong reference to
mGeckoContentController, so it can create use-after-free bugs where
they couldn't exist previously.

I think this is OK because TaskThrottler is scoped to APZC, which also
holds a strong ref to mGeckoContentController, and
mGeckoContentController is effectively immutable.  But this deserves a
second, close, look.

>diff --git a/gfx/layers/ipc/TaskThrottler.cpp b/gfx/layers/ipc/TaskThrottler.cpp

This seems mostly OK, but two issues

 - It would be a lot simpler to use

  nsAutoPtr<CancelableTask> mQueuedTask;

 - please document the memory ownership model at the TaskThrottler
   boundary

Would like to see the next version.

Comment 16

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> The task created here doesn't hold a strong reference to
> mGeckoContentController, so it can create use-after-free bugs where
> they couldn't exist previously.

RunnableMethod retains a counted reference to the callee at http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/task.h#299

Comment 17

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

Attachment: Throttle APZC paint requests v2

Comment 18

[Mozilla RelEng Bot]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65f06a1e7b1b

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e96fc511b960

https://tbpl.mozilla.org/php/getParsedLog.php?id=17612946&tree=Mozilla-Inbound

Comment 21

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

Attachment: Fix flash of blur on viewport change

Comment 22

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

Comment on attachment 689038 [details] [diff] [review]
Fix flash of blur on viewport change

Did you attach this to the bug you meant to?

Comment 23

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> Did you attach this to the bug you meant to?

Yes.

I can reproduce a full screen flash here http://people.mozilla.com/~nhirata/html_tp by pulling the screen up until the URL bar disappears then flicking down. When the URL bar reappears you get a flash which turns out to be a single frame that is rendered at the wrong zoom.

Throttling of paint requests helps reduce scrolling to unpainted regions (i.e. white screen) which was as close to reproducing the bug as I could get at nytimes.com. I managed to properly reproduce the issue while looking at bug 804808.

Comment 24

[Mozilla RelEng Bot]

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

Comment 25

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

Attachment: Throttle APZC paint requests v3

Removed broken MOZ_ASSERT.

Comment 26

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

Attachment: Fix flash of blur on viewport change v2

Fixed build break on Windows

Comment 27

[Mozilla RelEng Bot]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1d3367ca5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/195b2469641e

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b1d3367ca5d
https://hg.mozilla.org/mozilla-central/rev/195b2469641e

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c8dcce69b350
https://hg.mozilla.org/releases/mozilla-aurora/rev/cda6f7d54674

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1d330fd4ec79
https://hg.mozilla.org/releases/mozilla-b2g18/rev/3dd36851bd7c

Comment 31

[Mozilla RelEng Bot]

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

Comment 32

[Mozilla RelEng Bot]

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

Comment 33

[Mozilla RelEng Bot]

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