937185 - Displayport issues after loading a page in a background tab

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Pan and Zoom, defect)

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce:
1) Open Metro Firefox
2) Load a tall page like http://daringfireball.com
3) Scroll down the page a ways (several screen-heights)
4) Right-click (long-tap) a link and choose "Open link in new tab"
5) Without switching tabs, scroll back upward on the original page

Actual results: The page does not repaint correctly while scrolling, and shows black regions instead.

Comment 1

[Matt Brubeck (:mbrubeck)]

Note: After step 4, make sure to wait long enough for the new page to load in the background tab.  You should see the page thumbnail if you open the tab strip.

I'm not sure whether it's related, but I notice that widget::winrt::APZController stores a global (rather than per-tab) mLastScrollOffset, which could cause it to drop some UpdateScrollOffset calls if two different tabs have the same scroll offset.

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch

Tested with bug 937200, I don't see any issues.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 832155 [details] [diff] [review]
patch

I thought of something else that might go wrong here.

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 832155 [details] [diff] [review]
patch

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

::: widget/windows/winrt/APZController.h
@@ +26,5 @@
>  
>  public:
> +  APZController() :
> +    mLastScrollId(0),
> +    mWidgetListener(nullptr)

Should these be in the opposite order to avoid warnings?

Comment 5

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 6

[Jim Mathies [:jimm]]

Attachment: patch v.2

mind taking this for a spin to see if you can reproduce still?

I'm testing off a base page on my site - 
http://www.mathies.com/mozilla/
and right-clicking on alice.html as the background tab.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 832301 [details] [diff] [review]
patch v.2

I can still reproduce the bug with this patch and bug 937200 applied.  I've been testing on http://daringfireball.net/ and right-clicking on any of the ★ permalinks to open in a background tab.

Interestingly, I get different incorrect painting when I scroll with mousewheel than when I scroll with touch.  With mousewheel, the content does not scroll at all but gets clipped instead.  The bug also seems to happen more reliably with mousewheel.

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch v.3

This fixes things. When we reflow, scroll ids can change after first paint. In NotifyLayersUpdated, we save parts of the new frame metrics structure but we don't transfer the the scroll id or the press shell id. If these two change, things break.

One thing I'm not sure of, in RequestContentRepaint we request a paint and then copy the new pres shell id over. This seems odd to me. We don't use the pres shell id in APZCCallbackHelper when updating, so it shouldn't make any difference. I'm updating it the same time I update the scroll id here. I think this line can come out -    

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1167

No commenting here so I'm not sure about this.

Comment 9

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

Comment on attachment 832990 [details] [diff] [review]
patch v.3

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

Minusing for now - I'd like you to try the alternate approach I describe below.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1319,5 @@
>      mFrameMetrics.mZoom.scale *= parentResolutionChange;
>      mFrameMetrics.mResolution = aLayerMetrics.mResolution;
>      mFrameMetrics.mCumulativeResolution = aLayerMetrics.mCumulativeResolution;
> +    mFrameMetrics.mPresShellId = aLayerMetrics.mPresShellId;
> +    mFrameMetrics.mScrollId = aLayerMetrics.mScrollId;

I've been thinking about this and while I'm fine with copying over the mPresShellId I'm not sure it makes sense to copy over the mScrollId. AIUI the only way this code will get hit is if a layer is getting reused (and the APZC on it is therefore also reused) for a different piece of content. I think in this case it makes more sense to just destroy the old APZC and create a new one, because we don't want to be transferring the old APZC's state (like fling state and such) to the new content.

Instead of these two lines, can you try the following change to APZCTreeManager? If that works then I would prefer doing that.

diff --git a/gfx/layers/composite/APZCTreeManager.cpp b/gfx/layers/composite/APZCTreeManager.cpp
index a947b93..24e408e 100644
--- a/gfx/layers/composite/APZCTreeManager.cpp
+++ b/gfx/layers/composite/APZCTreeManager.cpp
@@ -116,16 +116,24 @@ APZCTreeManager::UpdatePanZoomControllerTree(CompositorParent* aCompositor,
       const CompositorParent::LayerTreeState* state = CompositorParent::GetIndirectShadowTree(aLayersId);
       if (state && state->mController.get()) {
         // If we get here, aLayer is a scrollable container layer and somebody
         // has registered a GeckoContentController for it, so we need to ensure
         // it has an APZC instance to manage its scrolling.
 
         apzc = container->GetAsyncPanZoomController();
 
+        // If the content represented by the container layer has changed (which may
+        // be possible because of DLBI heuristics) then we don't want to keep using
+        // the same old APZC for the new content. Null it out so we run through the
+        // code to find another one or create one.
+        if (!apzc->Matches(ScrollableLayerGuid(aLayersId, container->GetFrameMetrics()))) {
+          apzc = nullptr;
+        }
+
         // If the container doesn't have an APZC already, try to find one of our
         // pre-existing ones that matches. In particular, if we find an APZC whose
         // ScrollableLayerGuid is the same, then we know what happened is that the
         // layout of the page changed causing the layer tree to be rebuilt, but the
         // underlying content for which the APZC was originally created is still
         // there. So it makes sense to pick up that APZC instance again and use it here.
         if (apzc == nullptr) {
           ScrollableLayerGuid target(aLayersId, container->GetFrameMetrics());

::: widget/windows/winrt/APZController.cpp
@@ +18,5 @@
>  #include "nsIInterfaceRequestorUtils.h"
>  #include "nsLayoutUtils.h"
>  #include "mozilla/TouchEvents.h"
>  
> +#define DEBUG_CONTROLLER 1

Local debugging change? Probably don't want to land this.

::: widget/windows/winrt/APZController.h
@@ +27,5 @@
>  public:
> +  APZController() :
> +    mWidgetListener(nullptr)
> +  {
> +  }

This bit probably belongs in a separate patch but I'm not going to quibble about it :)

Comment 10

[Jim Mathies [:jimm]]

Ok will do. I'm also going to move the apzc.js changes over to bug 937750.

Comment 11

[Jim Mathies [:jimm]]

Attachment: patch v.4

Seems to be working.

Comment 12

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

Comment on attachment 8333819 [details] [diff] [review]
patch v.4

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

Thanks!

Comment 13

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/0b3e4b7cede2

thanks for the help!

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0b3e4b7cede2