1359844 - Ensure border-image-source be full-decoded after download

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

[u459114]

Separate from Bug 1341703, so that I can land that bug first.

Comment 1

[u459114]

Attachment: Bug 1359844 - Handle nsStyleBorder::CalcDifference off main thread.

Image loading status can be accessed in main thread only, so we can not use
nsStyleImage::IsLoaded in nsStyleBorder::CalcDifference, which might be executed
on a background thread.

The drawback of this change is we may paint a border-image, once, even before it
was actually loaded, which is a waste, because we have nothing to draw yet.

PS:
We did not check nsStyleImage::IsLoaded in both background-image and mask-image's
CalcDifference.

Review commit: https://reviewboard.mozilla.org/r/133974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133974/

Comment 2

[u459114]

Attachment: Bug 1359844 - Ensure full decode be triggered after style-image was downloaded,

In nsCSSRendering::PaintBorderWithStyleBorder:
DrawResult
nsCSSRendering::PaintBorderWithStyleBorder()
{
  if (aStyleBorder.IsBorderImageLoaded()) {
    (1)
  }

  (2)
}

At (1), we create a nsCSSBorderImageRenderer object. While creating it, we call
imgRenderer.PrepareImage() to ensure a full image decode at [1]. PrepareImage is
used in both bg-image/mask-image and border-image. The difference is, in
bg-image/mask-image painting path, we unconditionally use PrepareImage(in
nsCSSRendering::PrepareImageLayer), whereas in border-image painting path we
use it only after border-image was downloaded. This difference does cause a
problem. After border image was downloaded, the decoder will not do full decoding
since we did not ask for it, so there will be no repaint notification and no
chance to paint border-image again.

In this patch, I try to align the behavior between bg-image/mask-image and
border-image: always call nsImageRenderer::PrepareImage.

This is a generic fix for both stylo-enable and stylo-disable build. We do not
find this problem in reftest is because we use SYNC_DECODE in reftest harness, which
hides this race condition. When dairy using firefox, if
nsCSSRendering::PaintBorderWithStyleBorder is called after border-image was
loaded, your program run into (1), border-image will be drawn correctly; In the
case that nsCSSRendering::PaintBorderWithStyleBorder is called before
border-image is loaded, your program run into (2), and you can only see
border-color.

[1]
https://hg.mozilla.org/mozilla-central/file/a6f35285bd1e/layout/painting/nsCSSRenderingBorders.cpp#l3598

Review commit: https://reviewboard.mozilla.org/r/133976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133976/

Comment 3

[Boris Zbarsky [:bzbarsky]]

Wouldn't we want to land _this_ first, so bug 1341703 doesn't need to work around it?

Comment 4

[u459114]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #3)
> Wouldn't we want to land _this_ first, so bug 1341703 doesn't need to work
> around it?
Like we had discussed in bug 1341703, this problem(border-image may not be decoded occasionally) is a generic bug for both stylo-enable and stylo-disable build. (Although the hit rate of stylo-enable build is much much hihger) That's the main reason that I think we should take this patch out of bug 1341703.

Here is the try result of bug 1341703 without the patch here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1b525481c6900717e62709b48b200db3d03c531&selectedJob=92244773
border-image reftests are located at R4 on "Linux x64 Stylo debug/opt". Retry 10 times.

I do not see intermittent failures on border-image reftest on stylo-disable build because of this timing issue either, which means we won't hit this timing issue on reftest harness(I guess this is because we always do SYNC-DECODE in reftest harness, but that's just a guess). So it's safe to land these two bugs separately.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> That's the main reason that I think we should take this patch out of bug 1341703.

I agree that we should land this separately from bug 1341703.

But do we not have stylo-enabled failures after bug 1341703 but without this fix that are due to this bug?

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8861997 [details]
Bug 1359844 - Ensure full decode be triggered after style-image was downloaded,

https://reviewboard.mozilla.org/r/133976/#review136946

Relying on side-effects from a constructor seems a bit too implicit for my taste, but with a comment it might be ok.

Thank you for the great commit message!

::: commit-message-35e38:29
(Diff revision 1)
> +In this patch, I try to align the behavior between bg-image/mask-image and
> +border-image: always call nsImageRenderer::PrepareImage.
> +
> +This is a generic fix for both stylo-enable and stylo-disable build. We do not
> +find this problem in reftest is because we use SYNC_DECODE in reftest harness, which
> +hides this race condition. When dairy using firefox, if

"dairy"?

::: layout/painting/nsCSSRendering.cpp:831
(Diff revision 1)
>      uint32_t irFlags = 0;
>      if (aFlags & PaintBorderFlags::SYNC_DECODE_IMAGES) {
>        irFlags |= nsImageRenderer::FLAG_SYNC_DECODE_IMAGES;
>      }
>  
>      Maybe<nsCSSBorderImageRenderer> renderer =

Let's add a comment above here that says that creating the border image renderer will request a decode, and that we rely on that happening.

Comment 7

[u459114]

Comment on attachment 8861997 [details]
Bug 1359844 - Ensure full decode be triggered after style-image was downloaded,

https://reviewboard.mozilla.org/r/133976/#review136946

> "dairy"?

daily

Comment 8

[u459114]

Comment on attachment 8861997 [details]
Bug 1359844 - Ensure full decode be triggered after style-image was downloaded,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133976/diff/1-2/

Comment 9

[u459114]

Comment on attachment 8861997 [details]
Bug 1359844 - Ensure full decode be triggered after style-image was downloaded,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133976/diff/2-3/

Comment 10

[u459114]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5)
> > That's the main reason that I think we should take this patch out of bug 1341703.
> 
> I agree that we should land this separately from bug 1341703.
> 
> But do we not have stylo-enabled failures after bug 1341703 but without this
> fix that are due to this bug?
Reftest won't get failure without this patch. But since mstange already reviewed this patch. I triggered autoland.
The current status is 
1. Bug 1341703 stylo part had been merged
   https://github.com/servo/servo/pull/16620
2. Bug 1341703 gecko part had trigger autoland request.
3. Bug 1359844 had trigger autoland request.

" Tree gecko is closed - retrying later. ", I will check the result tomorrow morning.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebf68ba9b098
Ensure full decode be triggered after style-image was downloaded, r=mstange

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ebf68ba9b098