Closed Bug 1203417 Opened 9 years ago Closed 1 year ago

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

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED INCOMPLETE
Tracking Flags:
Tracking Status
firefox43 --- wontfix

People

(Reporter: nigelb, Unassigned)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

MozReview Request: Bug 1203417. Don't report BAD_IMAGE failure when trying to paint a null image. r=seth
58 bytes, text/x-review-board-request
seth
: review+
Details
MozReview Request: Bug 1203417. Propagate error result from PaintTableFrame. r=seth
58 bytes, text/x-review-board-request
seth
: review+
Details 
      No description provided.
[Mass Closure] Closing Intermittent as a one off
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
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.
Sorry, I'm not making any progress here.

:roc -- Can you have a look?
Assignee: gbrown → nobody
Flags: needinfo?(roc)
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 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)
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Severity: normal → S3

https://wiki.mozilla.org/Bug_Triage#Intermittent_Test_Failure_Cleanup
For more information, please visit auto_nag documentation.

Status: REOPENED → RESOLVED
Closed: 9 years ago → 1 year ago
status-firefox43: affected → wontfix
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: