Closed Bug 1624532 Opened 5 years ago Closed 5 years ago

Product image on sephora.com does not appear unless hovering over it

Categories

(Core :: Graphics: WebRender, defect, P2)

69 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla77
Tracking Status
firefox-esr68 --- unaffected
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- verified

People

(Reporter: ke5trel, Assigned: tnikkel)

References

(Blocks 1 open bug, Regression, )

Details

(Keywords: correctness, regression)

Attachments

(6 files, 2 obsolete files)

Attached image Product image is missing (deleted) —

STR:

  1. Visit: https://www.sephora.com/product/squalane-probiotic-gel-moisturizer-P416561
  2. Note the blank space for the product image.
  3. Hover cursor over blank space to make image appear.

Image should appear without hovering like with WebRender disabled.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1853d9e96a0990b01d856be8cdd67b49a9101eee&tochange=476956cf7d2e926a99b0c582550c286e3d8d44eb

Regressed by Bug 1553295.

Hi, thanks for the regression range! Would you also be able to attach a copy of your about:support?

That link unfortunately just takes me to the homepage, and if I click on a different product I cannot reproduce. Is it only on that product's page this occurs, or any product on that website?

Flags: needinfo?(a.beingessner)
Priority: -- → P3
Attached file about:support (deleted) —
(In reply to Jamie Nicol [:jnicol] from comment #1) > Hi, thanks for the regression range! Would you also be able to attach a copy of your about:support? > > That link unfortunately just takes me to the homepage, and if I click on a different product I cannot reproduce. Is it only on that product's page this occurs, or any product on that website? Hey. I originally posted about this in the nightly builds forum. This bug happens throughout the whole Sephora website. I have attached my about:support info for you.

(In reply to Jamie Nicol [:jnicol] from comment #1)

Hi, thanks for the regression range! Would you also be able to attach a copy of your about:support?

That link unfortunately just takes me to the homepage, and if I click on a different product I cannot reproduce. Is it only on that product's page this occurs, or any product on that website?

Hey. I originally posted about this in the nightly builds forum. This bug happens throughout the whole Sephora website. I have attached my about:support info for you.

Attached file index.html (deleted) —

I can reproduce the issue on Nightly76.0a1 Windows10 if WebRender is enabled.

With the attached html,
WebRender is enabled : the page is blank.
WebRender is disabled : the page shows an image.

Attached file index.html (deleted) —

Weirdly, this seems to work fine for me in a debug build but not an opt one.

It looks like what's going on here is that the "display:none" makes it so that we have no mask frame which is means we don't successfully draw a mask this is supposed to cause to not mask the item but we end up using an empty mask instead.

Layers avoids this by having IsMaskResourceReady(mFrame) return false in this case.

ooh, a cross platform webrender bug in 2020, neat!

Assignee: nobody → a.beingessner
Flags: needinfo?(a.beingessner)
OS: Unspecified → All
Priority: P3 → P2
Hardware: Unspecified → All

This comment explains a bit more about the situation:
https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/layout/svg/nsSVGIntegrationUtils.cpp#583

We basically want to do something similar in the BuildWrMaskImage path. i.e. detect if we're not going to get a mask and don't mask if we're in the SVG case.

This seems to cleanly implement the behaviour we want, try build here to check for situations I'm not anticipating: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f75631c932e50ca5203e7113945b403930382c3f

going on leave for a bit, so some quick notes:

  • the try run demonstrates a few things that break with my patch; it's too aggressive
  • Alice's attached example is the great start of a reftest
  • we need to clearly understand exactly under what circumstances "invalid" masks fail to "show everything" or "show nothing" (it's Weird)

interesting failing tests with my patch:

links to some context on how invalid masks are Weird:

At some point in one of yesterday's builds the Sephora website started working correctly again and images are now displaying as they should.

The wpt reftest hangs. It seems to be looping because isMozAfterPaintPending stays true when we have a mask that is display:none
isMozAfterPaintPending
https://searchfox.org/mozilla-central/rev/4e228dc5f594340d35da7453829ad9f3c3cb8b58/testing/marionette/listener.js#1815

So the infinite paints are caused in part by synchronous image decoding.

We make the draw result NOT_READY here
https://searchfox.org/mozilla-central/rev/9120151ddb35f2d4c37bfe613a54a4f10a9a3dc5/layout/svg/nsSVGIntegrationUtils.cpp#509
if there is no frame for the mask (it's display none), which means sync decode keeps trying to invalidate to try to get a successful draw.

Spun off the infinite paints in the test issue into bug 1628988 (with proper diagnosis and hacky patch).

(In reply to tnowens from comment #15)

At some point in one of yesterday's builds the Sephora website started working correctly again and images are now displaying as they should.

Not the case for me in latest Nightly, behavior is still the same.

PaintMaskSurface shouldn't be applying ImgDrawResult::NOT_READY when we don't have a frame and the mask image hasn't been resolved. ImgDrawResult is only about drawing images, not about waiting for external resources to resolve or frames to get constructed. The only purpose of tracking ImgDrawResult's in painting code is to know which frames we need to invalidate because their rendering might change if we sync decode images during a Draw call. Applying NOT_READY here means we invalidate for every paint with the sync decode images flag (ie reftest paints), and it never changes from NOT_READY. This bites the reftest for this bug 1624532.

To fix it, instead of "overloading" the ImgDrawResult we return a bool to indicate the mask is missing or incomplete.

Comment on attachment 9139917 [details]
Bug 1624532. Don't apply ImgDrawResult::NOT_READY in PaintMaskSurface. r?mstange

Revision D70595 was moved to bug 1628988. Setting attachment 9139917 [details] to obsolete.

Attachment #9139917 - Attachment is obsolete: true

The basic problem here for the page is that we should draw an svg element as if it has no mask specified if the specified mask is display: none. (For html elements in the same situation we should not draw the html element at all.)

The fix is to treat the return values of PaintMaskSurface (which come through nsSVGIntegrationUtils::PaintMask and nsDisplayMasksAndClipPaths::PaintMask) in WebRenderCommandBuilder::BuildWrMaskImage the same way as in CreateAndPaintMaskSurface.

Assignee: a.beingessner → tnikkel
Pushed by tnikkel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dab0d23256d7 Handle incomplete masks on svg content properly with webrender. r=mstange,jrmuizel
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/22945 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Upstream PR merged by moz-wptsync-bot

The patch landed in nightly and beta is affected.
:tnikkel, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(tnikkel)
Depends on: 1628988
Flags: needinfo?(tnikkel)

We've had this bug for a while and this would also require to uplift bug 1628988 which is a little more complicated, so I think we let this ride the trains.

Flags: qe-verify+

Reproduced the issue using firefox 76.0a1 (20200324214641) on Windows 10x64 using WebRender.
The issue is verified fixed with Firefox 77.0b3 (20200507233245) with WebRender on Windows 10x64 and macOS 10.12. The images are correctly shown when visiting the page from comment 0 and the attached test cases from comment 4 and comment 5.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Attachment #9136777 - Attachment is obsolete: true
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: