1201529 - When APZ is enabled, all browser.js codepaths that set a resolution or displayport should be disabled

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

STR to reproduce a specific problem:
1. Start a fennec instance built with --enable-android-apz
2. Open about:config
3. Put focus in the search field at the top

Actual:
Page gets zoomed out. You may need to un-focus and re-focus the search field to see this if it doesn't happen the first time.

Expected:
Page doesn't get zoomed out

I added some dump((new Error()).stack) calls in browser.js and got a bunch of different call stacks to code that is still calling setResolutionAndScaleTo. e.g.:

setResolution Tab.prototype.setDisplayPort@chrome://browser/content/browser.js:3835:38
Tab.prototype.sendViewportUpdate@chrome://browser/content/browser.js:4006:7
Tab.prototype.handleEvent@chrome://browser/content/browser.js:4369:9

That one is from the MozScrolledAreaChanged event listener.

setResolution Tab.prototype.setDisplayPort@chrome://browser/content/browser.js:3835:38
Tab.prototype.sendViewportUpdate@chrome://browser/content/browser.js:4006:7
viewportSizeUpdated@chrome://browser/content/browser.js:4742:5
ViewportHandler.observe@chrome://browser/content/browser.js:6281:11

That one is from the "Window:Resize" event handler.

Basically, once the C++ APZ is enabled, browser.js should not be making any calls whatsoever to setResolutionAndScaleTo, or to setDisplayPortMarginsForElement. If it does they will just interfere with what APZ is doing, and produce undefined (i.e. buggy) results.

I think the best path forward here is to disable all of these calls when APZ is enabled, and then look at them one by one to see if they result in broken features or behaviours. If so, we should implement that feature or behaviour in C++ code.

As an example, consider the code in the before-first-paint handler in browser.js. There is a bit of code there that adjusts the initial zoom in the case where we are loading an image or SVG document as the top-level document, and zooms it to fit the screen. After we disable the setResolution code in browser.js, this will probably stop working, and we will need to implement the desired behaviour in MobileViewportManager.cpp.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

What I had in mind.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Randall pointed out that this causes my griddiv.html page to not render at all when it is loaded. I can reproduce this misbehaviour, and it happens because the RCD doesn't get scrollable metrics. This in turn is a "regression" (using the term loosely) from bug 1198839, which stopped us from setting a displayport on the RCD-RSF. On Fennec the metrics with the RCD flag is special because that's what we sync to Java-land, and not having such a layer will cause problems.

In retrospect though I think the RCD-RSF is special (and not just on Fennec) because it needs to have scrollable metrics in order to get an APZC, and it needs an APZC in order for zooming to work. In the B2G parent process we don't care about this because there is a meta-viewport tag that disallows zooming but I suspect that if somebody modified the meta-viewport tag to allow zooming, it would be broken now with bug 1198839. Botond, what do you think? Should we back out bug 1198839?

Another alternative is to #ifdef it for Fennec so that we do always set a displayport on the RCD-RSF but I feel like that's less correct.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Some discussion on IRC starting at http://logs.glob.uno/?c=mozilla%23apz&s=4%20Sep%202015&e=4%20Sep%202015#c14516

I'm going to try and find more use cases that will help inform the right decision here.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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

Dropping review for now.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I gave this some thought and agree with botond that in the long term the ideal state would be:
(1) the root document's root element always has a displayport (to "cover" the entire tree)
(2) the RCD's root element only gets a displayport/layerized if it is scrollable or zoomable

In addition, this means that:
(3) the AsyncCompositionManager code for fennec which calls setFirstPaintViewport with the RCD's metrics should just use the root scrollable metrics on the tree instead, because there may not be a RCD at all
(4) this therefore means that Java code should not rely on getting metrics corresponding to the RCD

(1) happens already. (2) half-happens (it happens if the layer is scrollable, but not if the layer is only zoomable). (3) needs to be done - my fix for bug 1201625 touched on this but is incomplete in that it is restricted to B2GDroid. For (4) I filed bug 1202020 as it is more involved.

Since tackling (4) will take some time, I think that in the short-term we need a temporary fix where we fix (2) properly and do a partial fix for (3) in that it uses the metrics on the RCD if it finds one, and if it doesn't it can fall back to the root metrics. That will allow this bug and bug 1201581 to land without having to fix (4), which I feel is somewhat important to unblock further work.

I'll give this some more thought over the weekend and post patches on Monday if I don't change my mind in the meantime.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Implement step 3 in comment 6

This is the partial step (3) from comment 6. That is, if there is a RCD layer, use that for sending metrics to Java-land. If there is no RCD layer, use the metrics from the root layer instead.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Implement step 2 in comment 6

This implements step (2) from comment 6, which is my interpretation of what we discussed on Friday. That is, if the RCD-RSF is zoomable, then it should return true from WantAsyncScroll() so that it can get layerized. Let me know if I misunderstood something.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Don't let browser.js fiddle with resolution/displayport if APZ is enabled

Note that this applies on top of the patch from bug 1201416 which I landed on fx-team just now. This prevents browser.js from interfering with APZ properties while APZ is enabled.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Try push including these patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bf75164c069

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8658287 [details] [diff] [review]
Part 1 - Implement step 3 in comment 6

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +1163,5 @@
>      // in Gecko and partially in Java.
>      wantNextFrame |= SampleAPZAnimations(LayerMetricsWrapper(root), aCurrentFrame);
> +    bool foundRoot = false;
> +    if (ApplyAsyncContentTransformToTree(root, foundRoot)) {
> +      MOZ_ASSERT(foundRoot);

This assertion is failing on B2G emulator tests; I'll have to figure out why...

Comment 12

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> > +      MOZ_ASSERT(foundRoot);
> 
> This assertion is failing on B2G emulator tests; I'll have to figure out
> why...

Doh. The flag is only set #ifdef MOZ_ANDROID_APZ, so the assertion should be also #ifdef'd. Updated try push with that corrected: https://treeherder.mozilla.org/#/jobs?repo=try&revision=985776e289af

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Implement step 3 in comment 6 (v2)

Comment 14

[Kartikaya Gupta (email:[e-mail])]

There's still three failures on B2G emulator. I've fixed the mochitest locally and am looking into the two reftest failures.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

The R1 failure seems to be a bug in MultiTiledContentClient's painting code, it's wrapping the painted content from end of the tile to the other end. When WantAsyncScroll() returns false we use a TiledContentClient instead of a MultiTiledContentClient which doesn't have this problem. I'll file a separate bug for that in a bit.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

And the R11 failure also seems to be a problem with using a MultiTiledContentClient vs a SingleTiledContentClient; the display list and layer dumps are identical with and without the patch. Specifically the MultiTiledContentClient uses the tiled drawtarget which is causing problems; if I disable the tiled drawtarget code the tests pass.

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8658651 [details] [diff] [review]
Part 1 - Implement step 3 in comment 6 (v2)

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ -647,5 @@
>      // bug 1036967 removed the (dead) call.
>  
>  #if defined(MOZ_ANDROID_APZ)
> -    bool rootContentLayer = metrics.IsRootContent();
> -#ifdef MOZ_B2GDROID

I'm glad to see this #ifdef go away :)

::: gfx/layers/composite/AsyncCompositionManager.h
@@ +127,5 @@
>  
>  private:
>    void TransformScrollableLayer(Layer* aLayer);
>    // Return true if an AsyncPanZoomController content transform was
>    // applied for |aLayer|.

// |aOutFoundRoot| is set to true if ... Used on Android only.

@@ +128,5 @@
>  private:
>    void TransformScrollableLayer(Layer* aLayer);
>    // Return true if an AsyncPanZoomController content transform was
>    // applied for |aLayer|.
> +  bool ApplyAsyncContentTransformToTree(Layer* aLayer, bool& aFoundRoot);

|aOutFoundRoot| please (and if I recall our previous discussions on this subject correctly, |bool*| instead of |bool&|).

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8658288 [details] [diff] [review]
Part 2 - Implement step 2 in comment 6

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

::: layout/base/ZoomConstraintsClient.cpp
@@ +218,5 @@
> +  // We only ever create a ZoomConstraintsClient for an RCD, so the RSF of
> +  // the presShell must be the RCD-RSF (if it exists).
> +  MOZ_ASSERT(mPresShell->GetPresContext()->IsRootContentDocument());
> +  if (nsIScrollableFrame* rcdrsf = mPresShell->GetRootScrollFrameAsScrollable()) {
> +    rcdrsf->SetZoomableByAPZ(zoomConstraints.mAllowZoom);

Do we need to trigger a layout? [1]

[1] http://logs.glob.uno/?c=mozilla%23apz&s=4%20Sep%202015&e=4%20Sep%202015#c14558

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #17)
> >    // Return true if an AsyncPanZoomController content transform was
> >    // applied for |aLayer|.
> 
> // |aOutFoundRoot| is set to true if ... Used on Android only.

Will fix.

> @@ +128,5 @@
> >    // Return true if an AsyncPanZoomController content transform was
> >    // applied for |aLayer|.
> > +  bool ApplyAsyncContentTransformToTree(Layer* aLayer, bool& aFoundRoot);
> 
> |aOutFoundRoot| please (and if I recall our previous discussions on this
> subject correctly, |bool*| instead of |bool&|).

Heh. I did it as bool* first but I guess I misremembered our previous discussion. I was under the impression that if I did bool* I would have to null-check it in the body which I didn't want to do. I can change it back though.

(In reply to Botond Ballo [:botond] from comment #18)
> > +  MOZ_ASSERT(mPresShell->GetPresContext()->IsRootContentDocument());
> > +  if (nsIScrollableFrame* rcdrsf = mPresShell->GetRootScrollFrameAsScrollable()) {
> > +    rcdrsf->SetZoomableByAPZ(zoomConstraints.mAllowZoom);
> 
> Do we need to trigger a layout? [1]
> 
> [1]
> http://logs.glob.uno/
> ?c=mozilla%23apz&s=4%20Sep%202015&e=4%20Sep%202015#c14558

Hm I guess we should, yes. :) I'll update this patch with that change and the fixes for the tests once I have those sorted out.

Comment 20

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> > @@ +128,5 @@
> > >    // Return true if an AsyncPanZoomController content transform was
> > >    // applied for |aLayer|.
> > > +  bool ApplyAsyncContentTransformToTree(Layer* aLayer, bool& aFoundRoot);
> > 
> > |aOutFoundRoot| please (and if I recall our previous discussions on this
> > subject correctly, |bool*| instead of |bool&|).
> 
> Heh. I did it as bool* first but I guess I misremembered our previous
> discussion. I was under the impression that if I did bool* I would have to
> null-check it in the body which I didn't want to do. I can change it back
> though.

My recollection of the outcome of our discussion was this:

  - Always use pointers for out-parameters, so it's syntactically obvious
    at the call site that the caller will modify the value.

  - Document (in a comment) whether or not the out-parameter can be null.

An alternative was "use a reference when it can't be null", but we preferred the above for the syntactic-obviousness reason.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Implement step 3 in comment 6 (v3)

Part 1 updated to address review comments, carrying r+

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Implement step 2 in comment 6 (v2)

Update part 2 which includes the code to trigger a repaint as well the test workarounds; I filed bug 1204076 for the root cause there.

Try push in progress at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f4f3acb8cb3

I also need to do one more test locally to make sure the repaint code is working as expected; once that's done I'll reflag for review.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8660080 [details] [diff] [review]
Part 2 - Implement step 2 in comment 6 (v2)

The SchedulePaint call works as expected, re-requesting review on the changes.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10892d37b755
https://hg.mozilla.org/integration/mozilla-inbound/rev/10cbae31c4b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ed4f8be6250

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/10892d37b755
https://hg.mozilla.org/mozilla-central/rev/10cbae31c4b7
https://hg.mozilla.org/mozilla-central/rev/3ed4f8be6250