1351015 - stylo: Assertion range increased to 8 for image-rect/background-draw-nothing-malformed-images.html

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Assertion range increased to 8 for image-rect/background-draw-nothing-malformed-images.html (in bug 1350714) because of a URL bug. It was pre-existing and probably -moz-image-rect made it discoverable. We should fix this URL bug and decrease this range to 6.

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1350714#c6

Comment 1

[u459114]

#01: mozilla::nsImageRenderer::PrepareImage() (/home/cjcool/repository/mozilla-central-2/layout/painting/nsImageRenderer.cpp:151 (discriminator 1))
#02: nsCSSRendering::PrepareImageLayer(nsPresContext*, nsIFrame*, unsigned int, nsRect const&, nsRect const&, nsStyleImageLayers::Layer const&, bool*) (/home/cjcool/repository/mozilla-central-2/layout/painting/nsCSSRendering.cpp:311$
)
#03: nsDisplayBackgroundImage::GetInitData(nsDisplayListBuilder*, nsIFrame*, unsigned int, nsRect const&, nsStyleBackground const*, nsDisplayBackgroundImage::LayerizeFixed) (/home/cjcool/repository/mozilla-central-2/layout/painting/$
sDisplayList.cpp:2897)
#04: nsDisplayBackgroundImage::AppendBackgroundItemsToTop(nsDisplayListBuilder*, nsIFrame*, nsRect const&, nsDisplayList*, bool, nsStyleContext*) (/home/cjcool/repository/mozilla-central-2/layout/painting/nsDisplayList.cpp:3159)
#05: nsFrame::DisplayBackgroundUnconditional(nsDisplayListBuilder*, nsDisplayListSet const&, bool) (/home/cjcool/repository/mozilla-central-2/layout/generic/nsFrame.cpp:2070)
#

Comment 2

[u459114]

It's not a stylo only bug. The reason is this HTML file use a melformed png
  background-image: -moz-image-rect(url(../backgrounds/malformed.png), 0, 16, 16, 0);

nsStyleImage::ComputeActualCropRect()
{
  nsIntSize imageSize;
  imageContainer->GetWidth(&imageSize.width);
  imageContainer->GetHeight(&imageSize.height);
  if (imageSize.width <= 0 || imageSize.height <= 0) {
    return false; << Return false, which make scense, since we can not get valid size from malformed.png
  }
}

nsImageRenderer::PrepareImage()
{
  bool success =
    mImage->ComputeActualCropRect(actualCropRect, &isEntireImage);
  NS_ASSERTION(success, "ComputeActualCropRect() should not fail here"); << of cause we will hit this assertion.
}

Comment 3

[Bobby Holley (:bholley)]

Yes, if it's the same assertion we had already (and just increasing the count), let's just annotate it and move on.

Comment 4

[u459114]

Attachment: Bug 1351015 - Not assuming nsStyleImage::ComputeActualCropRect always return true.

ComputeActuralCropRect should not return false when the size of downloaded image
is empty, since the intrinsic size of an image can be empty. In this case, we
just need to make sure returning an empty crop-rect to tell the client: Yes, I had
downloaded and processed this image well, but the crop-rect is empty.

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

Comment 5

[u459114]

Comment on attachment 8860504 [details]
Bug 1351015 - Not assuming nsStyleImage::ComputeActualCropRect always return true.

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

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8860504 [details]
Bug 1351015 - Not assuming nsStyleImage::ComputeActualCropRect always return true.

https://reviewboard.mozilla.org/r/132514/#review136000

r=me with that assertion changed back to null check and return false.

::: layout/style/nsStyleStruct.cpp:2346
(Diff revision 2)
>  
>  bool
>  nsStyleImage::ComputeActualCropRect(nsIntRect& aActualCropRect,
>                                      bool* aIsEntireImage) const
>  {
> -  if (mType != eStyleImageType_Image) {
> +  MOZ_ASSERT (mType == eStyleImageType_Image,

Nit: no space before "(".

::: layout/style/nsStyleStruct.cpp:2350
(Diff revision 2)
>    imgRequestProxy* req = GetImageData();
> -  if (!req) {
> -    return false;
> +  MOZ_ASSERT(req,
> +             "req can not return null since mType is eStyleImageType_Image");

I think this assertion will not always be true.  For example, nsStyleImageRequest::Resolve might have just returned true because we have a local reference (and so this isn't really a valid image).  Also, if the URL was invalid in some way, ImageValue::Initialize will get null when it calls GetURI() (to pass it into ImageLoader::LoadImage), resulting in there being no imgRequestProxy for nsStyleImageRequest::Resolve to pull off the css::ImageValue.

Comment 7

[u459114]

Comment on attachment 8860504 [details]
Bug 1351015 - Not assuming nsStyleImage::ComputeActualCropRect always return true.

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

Comment 8

[u459114]

Comment on attachment 8860504 [details]
Bug 1351015 - Not assuming nsStyleImage::ComputeActualCropRect always return true.

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

Comment 9

[u459114]

Comment on attachment 8860504 [details]
Bug 1351015 - Not assuming nsStyleImage::ComputeActualCropRect always return true.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132514/diff/4-5/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6cf679052198
Not assuming nsStyleImage::ComputeActualCropRect always return true. r=heycam

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cf679052198