All users were logged out of Bugzilla on October 13th, 2018

[B2G][Marketplace][Preview Page] App page preview images do not show full screen other than the first and last

RESOLVED FIXED in Firefox 30, Firefox OS v1.3

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gbennett, Assigned: roc)

Tracking

({regression})

28 Branch
mozilla30
ARM
Gonk (Firefox OS)
regression
Points:
---

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: dogfood1.3, URL)

Attachments

(7 attachments)

(Reporter)

Description

5 years ago
Description:
When selected they just show up white, but you can still slide them left or right to get to the 1st and 4th images which do show.

Repro Steps:
1) Updated Buri 1.3 mozRIL to BuildID: 20140131004001
2) Connect data or wireless
3) Launch Marketplace and search for AccuWeather
4) Select AccuWeather app page and press the second and third preview images

Actual:
Images do not load and screen is shown white.

Expected:
Images load and work properly.

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140131004001
Gaia: 0ddcd8da5bfe1b48c73502ef29220e92f2db6b73
Gecko: 32e45047b663
Version: 28.0a2
Firmware Version: v1.2-device.cfg

Notes: The X on the top right to close the unloaded images does not work as well, the user needs to scroll to the first or fourth image to close out of the image preview slideshow.

Repro frequency: 100%, 2/2
See attached: http://www.youtube.com/watch?v=iTx2TpxBrq8
(Reporter)

Comment 1

5 years ago
This does not repro on 1.2

Environmental Variables:
Device: Buri 1.2 MOZ
BuildID: 20140128004004
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: d10e1f965d0c
Version: 26.0
Firmware Version: v1.2-device.cfg
Not a marketplace bug.
Component: Consumer Pages → General
Product: Marketplace → Firefox OS
Version: 1.3 → unspecified

Updated

5 years ago
Component: General → ImageLib
Product: Firefox OS → Core
Version: unspecified → 28 Branch

Updated

5 years ago
blocking-b2g: --- → 1.3?
Keywords: regression, regressionwindow-wanted
(Reporter)

Comment 3

5 years ago
Updated STR:

Repro Steps:
1) Updated Buri 1.3 mozRIL to BuildID: 20140131004001
2) Connect data or wireless
3) Launch Marketplace and search/select any app
4) Select any preview images except the first and last

Notes: I went through and checked a dozen other apps and it seems that other than the first and last images, no others can be viewed full screen.
Summary: [B2G][Marketplace][Preview Page] AccuWeather app page preview images 2 & 3 do not load when selected → [B2G][Marketplace][Preview Page] App page preview images do not show full screen other than the first and last

Updated

5 years ago
QA Contact: pfield

Comment 4

5 years ago
Wil, can you take a look if this is marketplace or not?   if not, maybe talk to graphics team.
Flags: needinfo?(clouserw)

Updated

5 years ago
blocking-b2g: 1.3? → 1.3+
It's hard to tell from the slim info available.  Would be great to see requests/responses and Error/JS console.  If the images are 404s or there is a JS error, it's our fault, but otherwise I don't think so.  I tried to reproduce on desktop and couldn't so my first reaction is that it isn't a Marketplace bug.  Let me know if I can be helpful.
Flags: needinfo?(clouserw)
qawanted for Wil's requests in comment #5.
Keywords: qawanted

Comment 7

5 years ago
This issue started to occur on the Buri 1.3 Build ID: 20140110004009

Environmental Variables:
Device: BURI 1.3 MOZ
BuildID: 20140110004009
Gaia: c606b129a2d1647c0fc7bfb197555026e9b27f9e
Gecko: c5109884ae3a
Version: 28.0a2
Firmware Version: V1.2-device.cfg

Last working Buri (branch) Build ID:

Environmental Variables:
Device: BURI 1.3 MOZ
BuildID: 20140109004002
Gaia: 22bc6be5b76cdc6d4e9667ff070979041a20ce2f
Gecko: 2c8f8683bd0d
Version: 28.0a2
Firmware Version: V1.2-device.cfg
Keywords: regressionwindow-wanted

Updated

5 years ago
Keywords: qawanted
This could possibly be related to bug 964517, or not, but I doubt it's ImageLib.
Let's say tentatively that this is APZC, as bug 907179 changes are in the regression range.
Component: ImageLib → Panning and Zooming
Whiteboard: dogfood1.3 → dogfood1.3, [ETA:2/21]

Comment 10

5 years ago
Milan, what's the meta bug for all APZC dependencies like these?
Blocks: 949585
Actually this is 1.3+ so i'll make it block gaia-apzc
Blocks: 909877
No longer blocks: 949585
Assignee: nobody → bugmail.mozilla
Status: NEW → ASSIGNED
I've been trying to figure out what's going on here but haven't made much progress. It only happens with APZC enabled. The position:fixed lightbox that shows the screenshots is set to width:100% and uses CSS transforms to display the appropriate one of the four images. The layer tree and displaylist dump seem normal as far as I can tell. If, using app manager, I reduce the width of the lightbox element, the images shrink to match, but still only the first screenful of them are rendered. I thought it might be an issue with the image-visibility prefs but even after hard-coding PresShell::AssumeAllImagesVisible() to return true the issue persists. I'll probably try creating a simplified test case to reproduce this.
Ah, making the lightbox not position:fixed makes all the images render. So definitely related to that.
Created attachment 8373681 [details]
Simplified test case

Here's a simplified test case that reproduces the problem. It works fine on desktop.
Actually after much poking and prodding this looks like a race condition. Even with the simplified test case sometimes it works fine I do really quick taps or longer taps but medium length taps don't work.
I think the timing thing I mentioned above is a bit of a red herring. I think the timing only affects when the displayport gets set on the document, which is an artifact of the reduced test case and shouldn't occur on the full site. However, in the reduced test case, commenting out the displayport-dependent chunk of code in ContainerState::FindFixedPosFrameForLayerData seems to fix the problem. I don't understand that code but I know it has changed between 1.3 and master. I will see if the code that's on master now fixes the problem; if not I'll have to come up with some questions to ask roc to understand the code, or hand it off to somebody else.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> commenting out the
> displayport-dependent chunk of code in
> ContainerState::FindFixedPosFrameForLayerData seems to fix the problem.

Specifically the new visible region that is set in that code seems to be the problem. roc suggested that the fixed-pos code in AsyncCompositionManager.cpp is wrong but I tried commenting out that stuff and it didn't help. If the problem is there then it's something that code should be doing (to the visible region) but isn't. I'm not convinced that's the case.
I still think the problem is in the code at [1]. Specifically I think there's some sort of coordinate system mismatch here. I loaded the simplified test case (attachment 8373681 [details]), and tapped on the text and broke into this function in gdb and looked around. At this point the relevant values are as follows:

(gdb) p displayPort
$1 = {
  <mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>> = {
    x = 0,
    y = 0,
    width = 19200,
    height = 21900
  }, <No data fields>}
(gdb) p aDrawRegion
$2 = (const nsIntRegion &) @0x43fbde60: {
  mImpl = {
    mRectCount = 3,
    mCurRect = 0x43f7619c,
    mRectListHead = {
      <nsRegion::nsRectFast> = {
        <nsRect> = {
          <mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>> = {
            x = 2147483647,
            y = 2147483647,
            width = 0,
            height = 0
          }, <No data fields>}, <No data fields>},
      members of nsRegion::RgnRect:
      prev = 0x43f7619c,
      next = 0x43f76334
    },
    mBoundRect = {
      <nsRect> = {
        <mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>> = {
          x = -1,
          y = -1,
          width = 390,
          height = 42
        }, <No data fields>}, <No data fields>}
  }
}
(gdb) p *aVisibleRegion
$3 = {
  mImpl = {
    mRectCount = 1,
    mCurRect = 0x43f7616c,
    mRectListHead = {
      <nsRegion::nsRectFast> = {
        <nsRect> = {
          <mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>> = {
            x = 2147483647,
            y = 0,
            width = 0,
            height = 0
          }, <No data fields>}, <No data fields>},
      members of nsRegion::RgnRect:
      prev = 0x43f7616c,
      next = 0x43f7616c
    },
    mBoundRect = {
      <nsRect> = {
        <mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>> = {
          x = 320,
          y = 19,
          width = 69,
          height = 22
        }, <No data fields>}, <No data fields>}
  }
}

After the displayport is adjusted by the offset to cross doc, it becomes:

(gdb) p displayPort
$4 = {
  <mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>> = {
    x = -480,
    y = -1440,
    width = 19200,
    height = 21900
  }, <No data fields>}

These values result in aVisibleRegion getting updated from from (320,19,69,22) to (-1,-1,210,42), which seems wrong to me. I think that in this code the displayport (after adjustment) is in a different coordinate space compared to aDrawRegion. One of these seems to be taking into account the CSS transform and the other does not, and when we "And" them together into the newVisibleRegion it comes out wrong.

roc, mattwoodrow, you guys know this code best - any suggestions? I will also attach output that shows the displaylist and layer tree dumps from this run in case it is useful.

[1] http://mxr.mozilla.org/mozilla-b2g28_v1_3/source/layout/base/FrameLayerBuilder.cpp?rev=9eaff8e0448d#1642
Flags: needinfo?(roc)
Flags: needinfo?(matt.woodrow)
Created attachment 8374874 [details]
Displaylist + layer dumps from the run in comment 18
Attachment #8374874 - Attachment description: Displaylist + layer dumps from the above run → Displaylist + layer dumps from the run in comment 18
What's the stacktrace to this happening?

As long as we're not inside an inactive layer, the all coordinates should be in post-transform space.

The numbers don't look too broken to me. We've made the visible region bigger to include everything that fits within the display port.
Flags: needinfo?(matt.woodrow)
The final state of the fixed-pos layer is
02-12 11:11:15.109 I/Gecko   (  522):     ClientThebesLayer (0x43598aa0) [transform=[ 1 0; 0 1; 8 8; ]] [visible=< (x=0, y=15, w=69, h=42); >] [opaqueContent] [isFixedPosition anchor=-8.000000,-8.000000 
margin=-1.000000,0.000000,0.000000,-1.000000] [valid=< (x=0, y=15, w=209, h=42); >]

The visible region (x=0, y=15, w=69, h=42) looks about right to me --- that should be about the dimensions and position of the word "SUCCESS" within the fixed-pos element.

But you're saying that in this testcase, the SUCCESS text isn't rendered properly/at all? It seems like we must have failed to paint it to the layer properly. Maybe enable the paint-dumping code in PaintInactiveLayer?
Flags: needinfo?(roc)
Created attachment 8375518 [details]
Backtrace to the visible region calculation

(In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> What's the stacktrace to this happening?

Attached.

> As long as we're not inside an inactive layer, the all coordinates should be
> in post-transform space.
> 
> The numbers don't look too broken to me. We've made the visible region
> bigger to include everything that fits within the display port.

But the visible region is also shifted over from being at x=320 to x=-1, and that's where the problem comes in. If I leave newVisibleRegion at the same size, but shift it back to x=320 y=19 (via newVisibleRegion.MoveBy(321,20)) then the text renders fine. Note that doing this doesn't seem to affect the final state of the fixed-pos layer that roc comments on; the visible region there remains the same.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> The final state of the fixed-pos layer is
> 02-12 11:11:15.109 I/Gecko   (  522):     ClientThebesLayer (0x43598aa0)
> [transform=[ 1 0; 0 1; 8 8; ]] [visible=< (x=0, y=15, w=69, h=42); >]
> [opaqueContent] [isFixedPosition anchor=-8.000000,-8.000000 
> margin=-1.000000,0.000000,0.000000,-1.000000] [valid=< (x=0, y=15, w=209,
> h=42); >]
> 
> The visible region (x=0, y=15, w=69, h=42) looks about right to me --- that
> should be about the dimensions and position of the word "SUCCESS" within the
> fixed-pos element.

Yup, that's correct.

> But you're saying that in this testcase, the SUCCESS text isn't rendered
> properly/at all?

Yes, and it seems directly related to the "newVisibleRegion" value computed.

> It seems like we must have failed to paint it to the layer
> properly. Maybe enable the paint-dumping code in PaintInactiveLayer?

Perhaps? When I enable the two sDumpPainting blocks in PaintInactiveLayer, I get the following additional output in logcat:

data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEUAAAAqCAYAAADlL5amAAAAIklEQVRoge3BgQAAAADDoPlTX+AIVQEAAAAAAAAAAAAA8A0tcgABkxHyogAAAABJRU5ErkJggg==

That looks pretty empty to me.
Flags: needinfo?(roc)
Flags: needinfo?(matt.woodrow)
roc is debugging this locally, but I think the problem is indeed that we're trying to set fixed position layer data on an inactive ThebesLayer.

I think this loop:

http://mxr.mozilla.org/mozilla-b2g28_v1_3/source/layout/base/FrameLayerBuilder.cpp#1636

Shouldn't be able to cross the reference frame boundary, otherwise we change coordinate space and it makes no sense.
Flags: needinfo?(matt.woodrow)
Yes, that's exactly right.
Flags: needinfo?(roc)
Created attachment 8376027 [details] [diff] [review]
fix

This is the simple, safe fix. I'll file a followup bug with additional fixes for other issues I found.
Assignee: bugmail.mozilla → roc
Attachment #8376027 - Flags: review?(matt.woodrow)
Created attachment 8376028 [details] [diff] [review]
Extra fix

Actually, let's take this as well, just to be sure.
Attachment #8376028 - Flags: review?(matt.woodrow)
I rebased these patches onto 1.3 and verified that they fix the original reported problem as well as the simplified test case. I tested part 2 by itself first and then applied part 1 as well - both seemed to be fine.
Component: Panning and Zooming → Layout
Attachment #8376027 - Flags: review?(matt.woodrow) → review+
Attachment #8376028 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/1cbbc597b7b5
https://hg.mozilla.org/mozilla-central/rev/4df228e4fa8b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Please request b2g28 approval on these patches as 1.3 blockers no longer have auto-approval for landing.
https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_4
Flags: needinfo?(roc)
Comment on attachment 8376028 [details] [diff] [review]
Extra fix

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Approval request is for both patches in this bug.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): APZC
User impact if declined: Bad rendering in Marketplace 
Testing completed: Not much
Risk to taking this patch (and alternatives if risky): Low risk. These patches were designed to be minimal, low-risk fixes.
String or UUID changes made by this patch: none
Attachment #8376028 - Flags: approval-mozilla-b2g28?
Flags: needinfo?(roc)

Updated

5 years ago
Attachment #8376028 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
ContainerState::FindFixedPosFrameForLayerData is a bit different on b2g28 and I'm not sure how to rebase for it. Please post a branch patch.
status-b2g-v1.3: --- → affected
status-b2g-v1.4: --- → fixed
status-firefox28: --- → wontfix
status-firefox29: --- → wontfix
status-firefox30: --- → fixed
Flags: needinfo?(roc)
Keywords: branch-patch-needed
Created attachment 8377861 [details] [diff] [review]
B2g28 part 1
Attachment #8377861 - Flags: checkin?
Flags: needinfo?(roc)
Attachment #8377861 - Flags: checkin?
Attachment #8377862 - Flags: checkin?
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/6e7588a3b89d
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/261e5eee90d7
status-b2g-v1.3: affected → fixed
Keywords: branch-patch-needed
Whiteboard: dogfood1.3, [ETA:2/21] → dogfood1.3
status-b2g-v1.3T: --- → fixed
You need to log in before you can comment on or make changes to this bug.