Intermittent layout/reftests/scrolling/fixed-table-1.html | image comparison (==), max difference: 165, number of differing pixels: 59976

REOPENED
Unassigned

Status

()

defect
P3
normal
REOPENED
4 years ago
3 years ago

People

(Reporter: nigelb, Unassigned)

Tracking

({intermittent-failure})

Trunk
Points:
---

Firefox Tracking Flags

(firefox43 affected)

Details

()

Attachments

(2 attachments)

Reporter

Description

4 years ago
No description provided.
Comment hidden (Legacy TBPL/Treeherder Robot)
[Mass Closure] Closing Intermittent as a one off
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Assignee: nobody → gbrown
This became more frequent recently, accounting for about 25% of Android intermittents last week.
Reftest analyzer shows the first (top, left) box is blank (all white) in the failed tests.
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Sorry, I'm not making any progress here.

:roc -- Can you have a look?
Assignee: gbrown → nobody
Flags: needinfo?(roc)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
I have reproduced this in rr chaos mode! \o/
Flags: needinfo?(roc)
The test fails because the first table's background image hasn't finished loading. At least, we receive the FRAME_UPDATE and FRAME_COMPLETE notifications after the reftest paints.
What happens here is that we try to paint the nsDisplayTableBackground before the image is fully loaded, without sync image decoding, and that sets its geometry's mLastDrawResult to BAD_IMAGE, which is wrong. It's set to BAD_IMAGE because nsTablePainter unconditionally calls nsCSSRendering::PaintBackgroundWithSC for various table parts such as col-groups, which have a default nsStyleBackground and nsStyleImageLayers; nsStyleImageLayers always constructs a single Layer of type eStyleImageType_Null (for reasons I do not know), and nsCSSRendering::PaintBackgroundWithSC tries to paint that layer, which ends up returning BAD_IMAGE. My fix for this bug is to make PaintBackgroundWithSC just skip layers of type eStyleImageType_Null.

For the record, after mLastDrawResult has been set to BAD_IMAGE, we later do a reftest paint with sync image decoding. nsDisplayTableItem::ComputeInvalidationRegion should invalidate the image due to aBuilder->ShouldSyncDecodeImages() && geometry->ShouldInvalidateToSyncDecodeImages(), but the latter is false because of mDrawResult being BAD_IMAGE. So we don't invalidate, and the table background isn't repainted at all.

There's another bug here I'm not fixing, which is that you could contrive a testcase where an image for a table part (e.g. a col-group) fails to load, causing BAD_IMAGE being stored for the entire nsDisplayTableBackground, which means we'd hit this same bug again. The underlying problem there is that nsDisplayTableBackground loads many images but we only store BAD_IMAGE for one image, so one bad image can cause us to skip invalidation for images that should still be sync-decoded.
Ah, there's another bug in TableBackgroundPainter::PaintTable: the result of PaintTableFrame is discarded. That also needs to be fixed to prevent this bug.
Comment hidden (Intermittent Failures Robot)
Comment on attachment 8718981 [details]
MozReview Request: Bug 1203417. Don't report BAD_IMAGE failure when trying to paint a null image. r=seth

https://reviewboard.mozilla.org/r/34821/#review33277

::: layout/base/nsCSSRendering.cpp:3007
(Diff revision 1)
> +        // XXX why does nsStyleImageLayers always allocate an empty Layer?

Yeah, it's worth thinking about this at some point. The problem with *not* reporting a null image as BAD_IMAGE is that otherwise, if the image remains null, we'll keep trying to paint it over and over and sync decoding for reftest snapshots will get stuck in an infinite loop. Basically the code as written now expects that an image being null is a permanent state, and apparently that's not the case...
Attachment #8718981 - Flags: review?(seth) → review+
Attachment #8718982 - Flags: review?(seth) → review+
Comment on attachment 8718982 [details]
MozReview Request: Bug 1203417. Propagate error result from PaintTableFrame. r=seth

https://reviewboard.mozilla.org/r/34823/#review33279
(In reply to Seth Fowler [:seth] [:s2h] from comment #31)
> Yeah, it's worth thinking about this at some point. The problem with *not*
> reporting a null image as BAD_IMAGE is that otherwise, if the image remains
> null, we'll keep trying to paint it over and over and sync decoding for
> reftest snapshots will get stuck in an infinite loop. Basically the code as
> written now expects that an image being null is a permanent state, and
> apparently that's not the case...

No. See comment #26; in this case null is a permanent state. The problem is that we're merging the results of painting a table background which is not null and loading with a col-group background which is null, to get BAD_IMAGE, which makes us fail to invalidate when the table background finishes loading.

> The problem with *not* reporting a null image as BAD_IMAGE is that otherwise, if the image remains null, we'll keep trying
> to paint it over and over and sync decoding for reftest snapshots will get stuck in an infinite loop.

In that case, perhaps this patch is incorrect ... but I don't see this behavior. For a null image we return SUCCESS, so why would we try to paint it over and over?
Flags: needinfo?(seth)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)

Comment 46

3 years ago
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.