Rocket chat [version 0.48.1] is not working properly

VERIFIED FIXED in Firefox 57

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: cladech_le_vrai, Assigned: dvander)

Tracking

({regression})

58 Branch
mozilla58
x86_64
Windows
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 verified, firefox58 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

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

Steps to reproduce:

I've been here : https://chat.lescommuns.org/channel/communecter_accueil

and it's not working properly


Actual results:

When you scroll, it takes a lot of time to display.


Expected results:

To display directly.
Reporter

Comment 1

2 years ago
It's probably because of the version of rocket chat 0.48.1 . But it's working on chrome for example.
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171030103605

Hello,

I have tested this issue on latest Nightly build and managed to reproduce it. I have accessed the provided link and scrolled the channel: the web painting isn't correctly rendered and also it takes a long time to display while scrolling.

[Regression]
Given the fact that this issue is not reproducible on latest Firefox Release 56.0.2, I've ran the Mozregression tool and managed to find a regression window.
Here are the results:
Last good revision: 8dba09333346ba9f445c6648622596a96a841e8d
First bad revision: 5bf42ef768d80ebeba9f0a634a38446c33f1c125
Pushlog: https://goo.gl/YNu5Va

From the pushlog, it seems that the bug 1408781 has caused this. David, can you please take a look?

This issue is not reproducible on Mac 10.12, nor on Ubuntu 14.04 x64, using latest Nightly build 58.0a1 (2017-10-30).
Blocks: 1408781
Status: UNCONFIRMED → NEW
Component: Untriaged → Graphics: Layers
Ever confirmed: true
Flags: needinfo?(dvander)
Keywords: regression
OS: Unspecified → Windows
Product: Firefox → Core
Hardware: Unspecified → x86_64
Okay, I can reproduce this. I don't think it's a regression from bug 1408781. It looks like it's been broken on Advanced Layers for a while, but bug 1408781 made it much more obvious. The site is not very smooth on the old D3D11 compositor but Advanced Layers definitely is clipping the invalid region incorrectly.
Assignee: nobody → dvander
Status: NEW → ASSIGNED
Flags: needinfo?(dvander)
Not related to bug 1408781 after all. Also not an invalidation bug since it happens with invalidation disabled.

What's happening is that there are two painted layers, A and B, with A sandwiched in between two visible strips of B. This means B has two rects as its visible region and A's visible region is between them:


    B B B B B B B
    A A A A A A A
    B B B B B B B     

A and B both are inside a container with FrameMetrics, which causes PaintedLayer::MayResample to return true, and that collapses B's visible region to one giant rect. Advanced Layers then adds that whole rect to the occlusion set, which makes A no longer visible.

There's two easy ways to fix this, one is to consider may-resample layers as never opaque. Another fix is to track the opaque and visible regions separately.
No longer blocks: 1408781
Posted patch patch (obsolete) — Splinter Review
Attachment #8925427 - Flags: review?(matt.woodrow)
Comment on attachment 8925427 [details] [diff] [review]
patch

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

::: gfx/layers/mlgpu/LayerMLGPU.h
@@ +83,5 @@
>    }
>  
> +  // If the layer is opaque, this returns the subset of the visible region
> +  // that can be considered opaque.
> +  virtual const LayerIntRegion& GetOpaqueRegion() {

Somewhat confusing that this is only valid to call if the layer is opaque.

Maybe return an empty region if !IsContentOpaque()? Or assert that we don't call this if it's false?

::: gfx/layers/mlgpu/PaintedLayerMLGPU.cpp
@@ +61,5 @@
>    // Note that when AL performs CPU-based occlusion culling (the default
>    // behavior), we might break up the visible region again. If that turns
>    // out to be a problem, we can factor this into ForEachDrawRect instead.
>    if (MayResample()) {
> +    mOpaqueRegion = Move(mRenderRegion);

Maybe just always write to mOpaqueRegion (before this branch)? That way GetOpaqueRegion can be a trivial getter, which makes this a bit simpler.
Attachment #8925427 - Flags: review?(matt.woodrow) → review+
Posted patch v2Splinter Review
Matt suggested not having two MayResample() calls since it's confusing. Here, PaintedLayerMLGPU always stores the correct visible region. RenderPassMLGPU then calls a separate method to extract the draw rects, and that's where we handle MayResample().

(I didn't make GetDrawRects virtual and replace all the callers of GetRenderRegion - but that's quite doable.)
Attachment #8925427 - Attachment is obsolete: true
Attachment #8925607 - Flags: review?(matt.woodrow)
Comment on attachment 8925607 [details] [diff] [review]
v2

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

::: gfx/layers/mlgpu/PaintedLayerMLGPU.cpp
@@ +65,5 @@
>    // Note that when AL performs CPU-based occlusion culling (the default
>    // behavior), we might break up the visible region again. If that turns
>    // out to be a problem, we can factor this into ForEachDrawRect instead.
>    if (MayResample()) {
> +    mDrawRects = mRenderRegion.GetBounds();

Do we need to store this? The bounds should already be cached as part of nsRegion, so I think we can just return mRenderRegion.GetBounds() directly here.
Attachment #8925607 - Flags: review?(matt.woodrow) → review+

Comment 9

2 years ago
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02dd65e55fd1
Don't treat resampled layers as entirely opaque. (bug 1412078, r=mattwoodrow)
Comment on attachment 8925607 [details] [diff] [review]
v2

Approval Request Comment
[Feature/Bug causing the regression]: Advanced Layers
[User impact if declined]: broken rendering on some complex pages, like RocketChat
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]: 
[Needs manual test from QE? If yes, steps to reproduce]: STR in comment #0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: The bug is that Advanced Layers considers too many pixels to be opaque, and thus fails to render pixels that should be visible. This just restricts how many are considered opaque, which worst case is a very small deoptimization but not a cause for further correctness problems.
[String changes made/needed]:
Attachment #8925607 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/02dd65e55fd1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8925607 [details] [diff] [review]
v2

I got a second opinion from Milan on this one and he feels this is a low risk, potential high reward. Taking it based on his recommendation, Beta57+
Attachment #8925607 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Reproduced on 2017-10-31 Nightly.
Verified as fixed using Firefox 57.0 build 3 and latest Nightly 58.0a1 2017-10-09 under Win 10 64-bit and Win 7 64-bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.