880932 - APZC: Page stops repainting and zoom/pan stops updating

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

Repainting stalls. Happens most of the time but not always. Steps to reproduce:

* Go to http://www.jesters-pies.co.nz/
* Zoom in
* Select menu
* Zoom in some more

Expected results:

Text gets bigger on zoom.

Actual results:

Text gets pixelated and is unreadable.

Comment 1

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

I'm confident that I know a good way to fix this. I will work on it on Monday.

Comment 2

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

Attachment: Fixed lack of response to APZC paint requests; paint requests get tagged with the current nsPresShellId. TabChild will

always trigger a response so APZC doesn't get left hanging.

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 760269 [details] [diff] [review]
Fixed lack of response to APZC paint requests;  paint requests get tagged with the current nsPresShellId. TabChild will

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

Sounds like we're just using this to work around a problem with the APZC. We should fix this in the right spot rather then force a ProcessUpdateFrame.

::: dom/ipc/TabChild.cpp
@@ +1473,5 @@
>      uint32_t presShellId;
>      nsresult rv = utils->GetPresShellId(&presShellId);
>      MOZ_ASSERT(NS_SUCCEEDED(rv));
>      if (NS_SUCCEEDED(rv) && aFrameMetrics.mPresShellId != presShellId) {
> +        return ProcessUpdateFrame(mLastMetrics);

The difference here is very subtle 'mLastMetrics vs aLastMetrics'. It's not clear if the pressShellId why you even want to ProcessUpdateFrame(mLastMetrics). This needs a clear comment.

Comment 4

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

(In reply to Benoit Girard (:BenWa) from comment #3)
> Sounds like we're just using this to work around a problem with the APZC. We
> should fix this in the right spot rather then force a ProcessUpdateFrame.

The problem is that we need to reply to every request otherwise APZC thinks that the content process is still painting. The way it is currently set up is that the replies are sent by the painting code.

Ideally we'd have a system where we continuously send updates to the content process and it would always process the most recent. Sadly we don't have the infrastructure to do that yet.

There is no direct messaging from TabChild to APZC. There may be some value in rethinking the messaging model. I'd prefer to do it when we're a little further along on the fennec stuff.

> The difference here is very subtle 'mLastMetrics vs aLastMetrics'. It's not
> clear if the pressShellId why you even want to
> ProcessUpdateFrame(mLastMetrics). This needs a clear comment.

Good point.

Comment 5

[Jinho Hwang [:jeffhwang]]

Attachment: Fixed blocked repainting requests and Zoom level mismatch

Comment 6

[Jinho Hwang [:jeffhwang]]

Hi Anthony, here is my small suggestion for this problem.

1. Passing updated PresShellID when it is changed on the same web page.
   
2. If it is needed we check and adjust the scale on UpdateZoomConstraints()
   when I try to zoom in on a bing.com page just right before APZC get UpdateZoomConstraints() called, I was able to zoom in/out the bing.com web page.

Comment 7

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

Comment on attachment 760811 [details] [diff] [review]
Fixed blocked repainting requests and Zoom level mismatch

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1275,5 @@
>      SetPageRect(mFrameMetrics.mScrollableRect);
>  
>      mState = NOTHING;
>    } else if (!mFrameMetrics.mScrollableRect.IsEqualEdges(aViewportFrame.mScrollableRect)) {
> +    mFrameMetrics.mPresShellId = aViewportFrame.mPresShellId;

We want to do this always. It could either be in NotifyLayersUpdated() or RequestContentRepaint(). Perhaps at the top of this function is best.

@@ +1477,5 @@
>                                                     float aMinZoom,
>                                                     float aMaxZoom) {
> + 
> +  float userZoom = mFrameMetrics.mZoom.width;
> +  if(aAllowZoom == false && aMaxZoom < userZoom || aMinZoom > userZoom) {

&& is higher precedence than || so this is equivalent to:

    (aAllowZoom == false && aMaxZoom < userZoom) || aMinZoom > userZoom

I'm not sure that is what is intended. The style convention would be !aAllowZoom instead of == false.

What problem are we trying to fix here? Is it a problem that could be better fixed elsewhere?

Comment 8

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

Attachment: Fixed lack of response to APZC paint requests; paint requests get tagged with the current nsPresShellId. TabChild will

always trigger a response so APZC doesn't get left hanging.

Comment 9

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

https://tbpl.mozilla.org/?tree=Try&rev=22f58f7511d7
https://tbpl.mozilla.org/?tree=Try&rev=6b7dc64f5f31

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/14cf0572b788

Comment 11

[Jinho Hwang [:jeffhwang]]

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #7)
> I'm not sure that is what is intended. The style convention would be
> !aAllowZoom instead of == false.
> 
> What problem are we trying to fix here? Is it a problem that could be better
> fixed elsewhere?

Thank you for your feedback.

Here is the problem and the steps to reproduce.
Problem : non-zoomable page can be zoom in/out before setting the constraints on zooming by UpdateZoomConstraints(). 
After this happened and changed aAllowZoom to false by the method, User are not able to adjust the zoomimg anymore badly. 

  Steps 
   1. open google.com
   2. zoom in on the page
   3. input bing.com on URL input box and click the arrow(->)
   4. before loading whole page, keep doing zoom in action on the screen.

  reproduce rate : 10% (I think it's kinds of timing error.)

For resolving this problem and make sure Bug 868047 are not reproducable anymore, Im suggesting the code.

Comment 12

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

(In reply to JinhoHwang from comment #11)
>   reproduce rate : 10% (I think it's kinds of timing error.)

This probably means that UpdateZoomConstraints() is happening after NotifyLayersUpdated(). That means that the zoom constraints are wrong at the point where we reset the zoom and we're still able to adjust it.

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/14cf0572b788

Comment 14

[Alex Keybl [:akeybl]]

Please renominate if we suspect that any major web properties are affected.

Comment 15

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

This bug is fairly annoying when it happens and it's not specific to this web page. It is an oversight in the fix for 868047 so I'd prefer to see either both or neither of these patches applied.

Comment 16

[Wayne Chang [:wchang]]

Anthony, can we have some risk assessment on this patch? Partner would like to see this on leo if the risk is low.

Hi Alex, renoming per comment 15 as usability and also partner concern.

Comment 17

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

The fix for 868047 eliminates a paint which is causing it to stop painting altogether. This fix makes it act more like it did before so I'm confident that it's a low risk fix. The down side is that you get an bit of unnecessary paint processing on pages that come up quickly. The extra processing is not significant.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/32621de9db5c

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/32621de9db5c