STR Open http://www.dailymail.co.uk/travel/article-2620529/The-luxurious-plane-world-offers-three-room-suite-personal-butler-gourmet-food-cost-12-500-ONE-WAY.html Click on the the large image. There will be a subtle bit of garbage on the first page. Swiping to the left will show a more visible bit of garbage. Looks like this might be the same height as the address bar?

What device? On my Nexus 4 running nightly I don't see that. The area above the image is solid black and if I scroll the URL bar off the screen it shows the image count (e.g. "2 of 10") with an "X" in the top-right corner.

I see this on my Galaxy S5. Scrolling a tad really demonstrates the behaviour. See YouTube: https://www.youtube.com/watch?v=rmvEoldnAMI

Hm. I suspect this might be related to other fixed-positioning bugs we have. The image layer is probably fixed-positioned and we are placing it too far down. Stuff on other layers that are now exposed may not have been painted and so we end up seeing garbage. Not sure though.

Device is a Nexus 5, Android 4.2.2

FYI, This bug does not happen on FFOS device (Flame) Gecko https://hg.mozilla.org/mozilla-central/rev/8be0e21fd300 │ │ BuildID 20140507160203 │ │ Version 32.0a1 │ │ ro.build.version.incremental=76 │ │ ro.build.date=Mon Apr 14 14:02:50 CST 2014

Interesting, scrolling down on this page reveals a fixed-position header that scrolls in at twice the rate of scroll position moving, suggesting that an async transform is being applied twice...

It seems like this might be an issue with nested fixed position layers? It's not totally clear, but that certainly seems to be the case.

So this error is not where I thought it was - removing the code that keeps fixed layers in place has no effect on this bug, meaning it's a result of the async scroll transform.

Wrong again, it seems to be a problem when applying the rendering offset - removing the async transform entirely (including the fixed layers transformation) still shows the bug.

I can confirm that this is caused by PrepareViewport being called twice on the same transform via CompositingRenderTargetOGL - just trying to figure out why that's happening and the correct fix.

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/8588817f7f86

Backed out by bug 1046017 comment 51.

Hi! I'm initial reporter for #1046017 on my old Samsung Ace 2 (I8160). Seems #1046017 fixed in 35 nightly. Checked this issue on our family's: Ace 2, S3, S4. On my smaaaall Ace2 it looks like not optimized for small screens, but I can see blank bar on scrolling up/down example from Comment 1. On Samsung S3 and S4 I see garbage on FF 32. S3 with 35 nightly (fixed #1046017) havn't garbage. S4 not tested with nightly. For me issue seems related to string "Image n of nn" on example page. It show/hide when tap on image there. ps. Sorry my Enlish :)

Milan, reopened because it was backed out. Need a new assignee

I can't reproduce the reason this was backed out on my Samsung Galaxy S2 unfortunately. Kats, any idea how this might have caused an issue with hit testing? The patch only appears to change the viewport transform, a clip rect within DrawQuad and the mOffset value of CompositingRenderTarget (which appears to only be accessed with DrawQuad and ContainerLayerComposite intermediate surface code). All of those changes seem to be entirely related to rendering (and mostly contained within CompositorOGL), so I don't see how hit testing could be affected by them. Does the java code do anything weird with processing touch events that could be affected by this?

I also don't have a device that could reproduce the hit testing issue. Without having such a device this will be tough to debug. Bad hit testing can occur any time gecko's rendering is out of sync with what's painted by the compositor. So if we are translating stuff in the compositor so that (0,0) is rendered at (0,50) but don't let gecko know in some way, then that will definitely result in bad hit testing - a tap at 50 pixels down the screen will visually be hitting CSS pixel 0 but gecko will interpret it as CSS pixel 50. In terms of the java event-transformation code, we do apply a translation due to the URL bar [1] state so it's possible that's not quite right. However the fact that this only appears on some devices and not others makes it unlikely that this code is the problem. It's more likely that the compositor-side code is failing on some drivers or something like that. Another alternative is that if we're only offsetting some of the layers and not others then the java code won't be taking that into account because from its point of view the entire content area is just one solid block. It would be up to gecko hit-testing code to deal with that sort of offsetting. [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/GeckoLayerClient.java?rev=d60356c72b78#967

:snorp, do you have access to Nexus 5, Android 4.2.2?

(In reply to Milan Sreckovic [:milan] from comment #20) > :snorp, do you have access to Nexus 5, Android 4.2.2? Nope.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19) > I also don't have a device that could reproduce the hit testing issue. > Without having such a device this will be tough to debug. Bad hit testing > can occur any time gecko's rendering is out of sync with what's painted by > the compositor. So if we are translating stuff in the compositor so that > (0,0) is rendered at (0,50) but don't let gecko know in some way, then that > will definitely result in bad hit testing - a tap at 50 pixels down the > screen will visually be hitting CSS pixel 0 but gecko will interpret it as > CSS pixel 50. > > In terms of the java event-transformation code, we do apply a translation > due to the URL bar [1] state so it's possible that's not quite right. > However the fact that this only appears on some devices and not others makes > it unlikely that this code is the problem. It's more likely that the > compositor-side code is failing on some drivers or something like that. > > Another alternative is that if we're only offsetting some of the layers and > not others then the java code won't be taking that into account because from > its point of view the entire content area is just one solid block. It would > be up to gecko hit-testing code to deal with that sort of offsetting. > > [1] > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/ > GeckoLayerClient.java?rev=d60356c72b78#967 Ok, that makes sense. Hit testing is working fine, but the compositor is just drawing the content in the wrong location. That fits with the description in Bug 1046017 Comment 7.

Well, I have absolutely no idea why this is device dependent. What I do know: * This bug was caused by us applying mRenderOffset to the viewport transform for all targets (including intermediate surfaces), moving the offset onto the CompositingRenderTarget for the window fixes that. * In Compositor::DrawQuad, aRect isn't already adjusted for the current CompositingRenderTarget's offset, but aClipRect *is* (this seems a bit weird) * The exception to this is the CompositingRenderTarget offset we added for mRenderOffset, because Layer::CalculateScissorRect only adjusts clip rects in the presence of intermediate surface which this isn't. * This is why removing the code to adjust aClipRect by mRenderOffset caused bug 1017427 * Bug 1072579 was caused by the exact opposite issue. We're rendering to an intermediate surface, so we shouldn't be moving the clip by mRenderOffset for that draw. The patch in this bug 'fixes' it, and the patch in bug 1017427 breaks it again. Without any way of reproducing the issues from bug 1046017 I don't think I can do much to tidy this up completely. I'll try come up with the minimal patch that fixes all the issue and hope it doesn't regress anything.

Please see previous comment :) fix-ogl-compositor-screen-render-offse

My patch fixes everything I could reproduce locally (this bug, bug 1017427, bug 1072579). Hopefully someone can confirm that it doesn't cause bug 1046017 again.

Comment on attachment 8498526 [details] [diff] [review] Fix v2 Review of attachment 8498526 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the two comments below addressed. ::: gfx/layers/opengl/CompositorOGL.cpp @@ +592,5 @@ > viewMatrix.PreScale(2.0f / float(aSize.width), 2.0f / float(aSize.height)); > viewMatrix.PreScale(1.0f, -1.0f); > } > > + if (!mTarget && mCurrentRenderTarget->IsWindow()) { Are we sure that mCurrentRenderTarget cannot be null here? Please explicitly assert it, or handle the null case. @@ +1020,5 @@ > > MOZ_ASSERT(mFrameInProgress, "frame not started"); > > Rect clipRect = aClipRect; > + if (!mTarget && mCurrentRenderTarget->IsWindow()) { this method appears to already use mCurrentRenderTarget without checking for nullptr. Please add an assertion at the beginning of the method.

Matt - This bug impacts Aurora. Can you please submit and Aurora approval request once you're confident that the fix is good on m-c?

We were never able to reproduce 1046017 in house. It was filed late in the beta cycle from the community and was only raised to a high level of importance after release. This makes me cautious about uplifting this fix.

(In reply to Kevin Brosnan [:kbrosnan] from comment #31) > We were never able to reproduce 1046017 in house. It was filed late in the > beta cycle from the community and was only raised to a high level of > importance after release. This makes me cautious about uplifting this fix. I checked in that bug, and got multiple confirmations that the new patch didn't reintroduce the issue. I think we should be ok to uplift this into Aurora.

Comment on attachment 8498526 [details] [diff] [review] Fix v2 Approval Request Comment [Feature/regressing bug #]: Unknown. [User impact if declined]: Invalid content shown, or invalid clipping applied (bug 1072579) to some content. [Describe test coverage new/current, TBPL]: Tested all known test cases manually, checked with reporters of previous regressions, passes all TBPL tests. [Risks and why]: Pretty low risk, less invasive than the previous fix. [String/UUID change made/needed]: None.

Comment on attachment 8498526 [details] [diff] [review] Fix v2 Matt, thanks for confirming that this patch doesn't reintroduce the issue in bug 1046017 (comment 32). I'm going to take this in 34. We have the entire beta cycle to see if any new reports come in about the issue. Aurora+

Sounds gtm.

Verified as fixed in Builds: Firefox for Android 34 Beta 1 Firefox for Android 35.0a2 (2014-10-15) Device: Motorola Razr (Android 4.1.2)