Closed Bug 1419225 Opened 5 years ago Closed 5 years ago

Flickering background on humblebundle.com

Categories

(Core :: Web Painting, defect)

Product:
Core
Component:
Web Painting
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: yoasif, Assigned: mattwoodrow)

References

Details

Attachments

(3 files)

flickering-humble.mp4
1.66 MB, video/mp4
Details
testcase.zip
4.48 MB, application/zip
Details
transform-clip-to-app-units
1.62 KB, patch
mikokm
: review+
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached video flickering-humble.mp4 — 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171120100042

Steps to reproduce:

1. Navigate to https://www.humblebundle.com/
2. Wait for page to load
3. Scroll down page


Actual results:

The background (made up of images of games) behind text:

Humble Care Package Bundle

Hurricanes. Wildfires. Earthquakes. In the wake of large-scale natural disasters, we're joined by an amazing group of developers to raise funds for emergency relief. 100% goes to charity, and Humble will match contributions up to $300,000.

flickers.


Expected results:

No flickering as on release. 

See video.
Blocks: 1352499
Has STR: --- → yes
status-firefox57: --- → unaffected
status-firefox59: --- → affected
Even with retained display list disabled, this site does not seem to work properly. The background scrolling is very laggy unless the mouse is hovered over it. This is very likely caused by a problem with 3D transform invalidation.
Regressed by bug 1190721.

20:52.71 INFO: Last good revision: 401840a241b9861cac205bd866ba24e69d11b7e2
20:52.71 INFO: First bad revision: fa94f7205173d34f23975c6af9cb95237b28c8b8
20:52.71 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=401840a241b9861cac205bd866ba24e69d11b7e2&tochange=fa94f7205173d34f23975c6af9cb95237b28c8b8
Depends on: 1190721
Flags: needinfo?(hikezoe)
When I open the site, "GET THE BUNDLE" is not shown up, so I can't confirm what's going on there.  It would be nice if someone provides reduced test cases.  Thanks!

Keep ni? to me.
Miko, would you mind providing downloaded version of the site?  The site seems to be somewhat different to me (because of accessing from Japan?)
Flags: needinfo?(hikezoe) → needinfo?(mikokm)
Attached file testcase.zip — 
Reduced test case from humblebundle.com
Flags: needinfo?(mikokm)
Thank you, Miko!  I take this.
Assignee: nobody → hikezoe
Hmm, the test case doesn't work even if dom.animations.offscreen-throttling is disabled..
See Also: → 1420284
(In reply to Hiroyuki Ikezoe (:hiro) (PTO on 11/24) from comment #6)
> Thank you, Miko!  I take this.

Hi Hiro, this test case actually exposed three separate bugs: the flickering background, texture corruption, and problem with animation throttling.
For clarity, I would like to preserve this bug for the flickering background with retained display lists. I will also file a new bug for texture corruption.

I've split the animation throttling part to bug 1420284.
Our rectangle representation is x/y/width/height, so when converting transformed corners into a rectangle, we have to be careful of the case where the difference between two edges is greater than the max signed value. If we don't handle this correctly, then we can end up accidentally clipping off the bottom/right edges of large rectangles.

We were trying to handle this with TransformAndClipBounds, but we were clipping to the range allowed by float, not what we're using for the final rect, nscoord.

The perspective transform was giving us a really large rectangle, which we then lost the bottom/right edge of, and it no longer intersected the overflow area.

This now matches what we do when computing the overflow using MatrixTransformRect.

Converting this page into a testcase would be nice, but it was pretty inconsistently failing for me.
Assignee: hikezoe → matt.woodrow
Attachment #8931540 - Flags: review?(mikokm)
(In reply to Miko Mynttinen [:miko] from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) (PTO on 11/24) from comment #6)
> > Thank you, Miko!  I take this.
> 
> Hi Hiro, this test case actually exposed three separate bugs: the flickering
> background, texture corruption, and problem with animation throttling.
> For clarity, I would like to preserve this bug for the flickering background
> with retained display lists. I will also file a new bug for texture
> corruption.
> 
> I've split the animation throttling part to bug 1420284.

It seems that flickering/disappearing background and animation throttling ended up being because of the same bug. I cannot reproduce either anymore after applying Matt's patch.
Comment on attachment 8931540 [details] [diff] [review]
transform-clip-to-app-units

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

LGTM.
Attachment #8931540 - Flags: review?(mikokm) → review+
CC'ing botond since this is a bug that only happens because of our current rectangle representation.

Switching to an x1/y1/x2/y2 representation would fix this, and remove the need to do weird clipping to a max-sized centered rectangle.

Just another data point for your project :)
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50ad1d2828bf
Clip transformed rectangles to the maximum allowed values for the appunit coordinates we're converting into. r=miko
https://hg.mozilla.org/mozilla-central/rev/50ad1d2828bf
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8931540 [details] [diff] [review]
transform-clip-to-app-units

Approval Request Comment
[Feature/Bug causing the regression]: bug 1352499. This is code that is preffed off, but we want to run a shield study enabling the pref.
[User impact if declined]: None, preffed off code.
[Is this code covered by automated tests?]: Yes, when the pref is enabled.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Code is preffed off.
[String changes made/needed]: None
Attachment #8931540 - Flags: approval-mozilla-beta?
Comment on attachment 8931540 [details] [diff] [review]
transform-clip-to-app-units

Support retain display lists shield study which is pref-off now. Beta58+.
Attachment #8931540 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have reproduced this bug with Nightly 59.0a1 (2017-11-20) on Windows 10, 64 Bit!

This bug's fix is verified with Latest Beta and Latest Nightly!

Build ID  : 20171130160223
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0


Build ID  : 20171201100115
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
QA Whiteboard: [bugday-20171129]
Depends on: 1468124
You need to log in before you can comment on or make changes to this bug.