1128756 - Do not call NotifyInvalidations when executing nsDisplayList::PaintRoot as a result of a call to canvas.drawWindow

Status: RESOLVED INVALID

(Core :: Graphics: Layers, defect)

Description

[Seth Fowler [:seth] [:s2h]]

So imagine that we're in a reftest, and we're trying to paint a decode-on-draw image that is within the viewport, but completely occluded by something else. Here's how things might unfold:

1. We're getting ready to take a reftest snapshot, and start constructing a display list for a paint.

2. In the display item for the image (let's say nsDisplayBackgroundImage), we see that we're supposed to be sync decoding images, and our image isn't decoded yet, so we invalidate our area to trigger sync decoding.

3. We paint, but we end up not painting anything for that display item since it's completely occluded, so the decode-on-draw image does not get painted and hence does not get decoded.

4. We send an invalidation notification. The reftest harness notices and decides to take a new snapshot, so we prepare to repaint into a canvas.

5. Just like in #2, when we start constructing the display list and get to our nsDisplayBackgroundImage, we see that we're supposed to be sync decoding. We didn't decode our image, because we didn't paint it, so we invalidate again.

6. Just like in #3, we paint, but we still don't paint our nsDisplayBackgroundImage since it's completely occluded.

7. Just like #4, we send an invalidation notification and the reftest harness decides to take a new snapshot.

8. (Repeat until the test times out.)

This issue makes decode-on-draw images very dangerous in reftests. Even worse, the new image sync decoding design I'm adding in bug 1128225 and its followups can also trigger this, because it makes the decision on whether to invalidate for sync decoding based upon what happened the last time we tried to paint for a frame, and if we decide not to paint the frame because it's occluded, we'll run into this issue there too. (That's actually how I noticed this: when updating nsDisplayBackgroundImage to use the new sync decoding approach, I ran into this issue on |layout/reftest/backgrounds/background-repeat-large-area.html|.)

In the long term, the ideal solution would be to rework things so that we know whether we're occluded in nsDisplayItem::ComputeInvalidationRegion. I'm not sure of the feasibility or magnitude of that change, but it seems pretty clear that it's not something we can do right away. Therefore, in the short term, let's take a simpler approach: just don't call NotifyInvalidations when we're executing nsDisplayList::PaintRoot as a result of a call to canvas.drawWindow. This prevents the loop above, and it does not seem like it could possibly have any negative effect on web content.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Do not call NotifyInvalidations when executing nsDisplayList::PaintRoot as a result of a call to canvas.drawWindow

Here's the patch. The actual meaningful changes are in
CanvasRenderingContext2D.cpp and nsDisplayList.cpp; the rest is just routing
flags from one to the other.

Comment 2

[Seth Fowler [:seth] [:s2h]]

OK, as I mentioned on IRC I've been investigating an alternate solution, and at this point it looks like it's going to work. (See bug 1128769 part 2 for the code.) I'm going to unset the review flag for now, and will resolve the bug once it's confirmed that bug 1128769 can land without this patch.

Comment 3

[Seth Fowler [:seth] [:s2h]]

We ended up not needing this. Resolving.