479387 - can hit "converted background area should not be empty" warning because of bug 472769

Status: NEW

(Core :: Web Painting, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think as a result of the patch in bug 472769:
  http://hg.mozilla.org/mozilla-central/rev/50e934e4979b
we can hit the warning "converted background area should not be empty", then in nsCSSRendering::PaintBackgroundWithSC, and now (moved by bug 322475) to SetupBackgroundClip (still in nsCSSRendering.cpp).

(In earlier versions of my patch in bug 322475, I had changed this into an NS_ABORT_IF_FALSE, which I think was valid then, but after bug 472769 is no longer valid.  When I landed last night or this morning, dolske had pulled a tree with the landing that I backed out, and he hit that abort.  When I relanded, I changed it back to match what the current code does.)


In particular, I think the problem is that that patch removed these lines:
-  dirtyRectGfx.Round();
-  dirtyRectGfx.Condition();
which means that the code inside if (aHaveRoundedCorners) that computes bgAreaGfx could end up with an empty bgAreaGfx even if aDirtyRectGfx (before bug 322475, dirtyRectGfx) is non-empty, since the rounding could make a non-empty rectangle into an empty one.


It seems to me that:

  (1) if it's ok to do the Round() and Condition() before the emptiness test, then perhaps we should (even if it's only for the emptiness test)

  (2) if it's not ok to do the Round() and Condition() before the emptiness test, then we're hitting whatever problems doing it causes, but only in the case where we have a border radius.

Comment 1

[Daniel Holbert [:dholbert]]

Do we know the conditions under which we've been hitting this warning?

It seems to me that the set of cases where Round() and Condition() would affect emptiness are pretty small:

The Round() call would only change the "IsEmpty()" status in a situation with:
   aDirtyRectGfx.y       >= [N].5px
   aDirtyRectGfx.YMost() <  [N+1].5px
In other words, the dirty height would have to be **less** than 1px, and the dirty y-value would need to be positioned such that adding the height doesn't make us reach a mid-pixel boundary.

The Condition() call would only change the "IsEmpty()" status when the x & y values beyond CAIRO_COORD_MAX or CAIRO_COORD_MIN.

Do we know which of those cases we're hitting?

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
>   (1) if it's ok to do the Round() and Condition() before the emptiness test,
> then perhaps we should (even if it's only for the emptiness test)

It should definitely be safe to add a Condition() call before the emptiness test, since that seems to just check for out-of-bounds values.  (It probably should just go in SetupDirtyRects, which I think has the code that previously surrounded the Condition() call that I removed in bug 472769).

I don't think it's ok to Round() the dirtyRect, though, *unless* we do something like RoundOut().  Otherwise, we ignore legitimately dirty space (particularly in the case of this bug here, with an empty dirtyRect that apparently becomes empty due to Round()ing).

If we just round for the purpose of the emptiness test, then I suppose that's safe.  I guess we'd need to do that in a temporary variable, though.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> Otherwise, we ignore legitimately dirty space
> (particularly in the case of this bug here, with an empty dirtyRect that
> apparently becomes empty due to Round()ing).

Sorry -- I meantto say "...with a NON-empty dirtyRect that apparently becomes empty due to Round()ing"