MozReftestInvalidate doesn't reliably test invalidation in first reftest (Full-window-repaint due to focus-in after MozReftestInvalidate)

NEW
Unassigned

Status

Testing
Reftest
6 years ago
6 years ago

People

(Reporter: dholbert, Unassigned)

Tracking

Trunk
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 632456 [details] [diff] [review]
patch that demonstrates this bug

STR:
 1. Apply the attached patch. (includes 2 reftests and an intentionally-broken code change)
       (the code change keeps block frames from reporting their
        kids' overflow areas in their own overflow areas.)

 2. Rebuild firefox.

 3. Run "make reftest" in your objdir.  (This just runs the 2 included reftsts.)

EXPECTED RESULTS: The two reftests run, and both fail.

ACTUAL RESULTS: Only the second reftest fails.

DISCUSSION:
The first (passing) reftest is a "conventional" MozReftestInvalidate-based test. When the MozReftestInvalidate event fires, it tweaks a margin and then removes reftest-wait.

The second (failing) reftest just adds a setTimeout (after MozReftestInvalidate fires), for some additional delay.

Because of the bug that this patch introduces in nsBlockFrame, both reftests *should* fail.  This indicates that MozReftestInvalidate isn't functioning as expected, I think.
(Reporter)

Comment 1

6 years ago
Created attachment 632459 [details]
reftest log (verbose)

Here's the terminal output from running "MOZ_REFTEST_VERBOSE=1 make reftest"

The "DoDrawWindow" calls are of particular interest.

> REFTEST INFO | START file:///scratch/work/builds/mozilla-central/mozilla-central.12-03-19.10-25/mozilla/layout/reftests/invalidate-test-1a.html
> REFTEST INFO | DoDrawWindow 0,0,800,1000
[...]
> REFTEST INFO | DoDrawWindow 0,0,800,1000
[...]
> REFTEST INFO | RecordResult fired
[...]
> REFTEST INFO | START file:///scratch/work/builds/mozilla-central/mozilla-central.12-03-19.10-25/mozilla/layout/reftests/invalidate-test-1b.html
[...]
> REFTEST INFO | DoDrawWindow 0,0,800,1000
[...]
> REFTEST INFO | DoDrawWindow 8,8,140,50
[...]
> REFTEST INFO | [CONTENT] RecordResult fired


So -- the first test gets two fullscreen paints, whereas the second test (with the setTimeout) gets a fullscreen paint and an incremental paint of a smaller region.

So, the first test isn't actually testing incremental invalidation/painting, as we'd expect it to.  We apparently need to use setTimeouts to get that right now.  That's bad.
(Reporter)

Comment 2

6 years ago
(Not sure whether the reftest harness itself is at fault here, or core:layout, or something else.  Feel free to reclassify the bug if you have a hunch.)
(Reporter)

Comment 3

6 years ago
Here's a Try build with this bug's patch attached:
  https://tbpl.mozilla.org/?tree=Try&rev=5e29d593fde5

So far...
 * Linux64 opt & debug both show ACTUAL RESULTS. (1st test incorrectly passes)
whereas...
 * OS X64 debug shows EXPECTED RESULTS (both tests fail, good)
 * OS X 10.7 debug shows EXPECTED RESULTS (both tests fail, good)
 * Other platforms are still in-progress.

So: I haven't encountered this on mac yet, & it could be a linux64-specific issue.
(Reporter)

Comment 4

6 years ago
FWIW, my testing of this (locally & in that try run) has been based off of this m-c push, from today:
 https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=d39501066aa4

I initially suspected DLBI might be involved, but DLBI was landed & backed out, so it's not in m-c at the moment and is hence unrelated. (side note: mattwoodrow tells me we'd need a different intentional-bustage-code-change to try to make these tests fail with DLBI, since DLBI doesn't use overflow rects for invalidation.)
Looks like only Linux is affected, fortunately.

Can you take this? You could try logging or setting breakpoints in nsFrame::InvalidateRoot to catch invalidations with x=0, and see where they're coming from.
(Reporter)

Comment 6

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Looks like only Linux is affected, fortunately.

Yeah, looks like it, from the TBPL results. Even Android failed both reftests (which is what we'd want), as did Linux32 debug.

The only platforms that (incorrectly) passed the first reftest were Linux32 opt, Linux64 opt, and Linux64 debug.

> Can you take this? You could try logging or setting breakpoints in
> nsFrame::InvalidateRoot to catch invalidations with x=0, and see where
> they're coming from.

Sure -- I'll look into it at some point in the next few days.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Reporter)

Comment 7

6 years ago
s/days/weeks/
(In reply to Daniel Holbert [:dholbert] from comment #1)
> So -- the first test gets two fullscreen paints, whereas the second test
> (with the setTimeout) gets a fullscreen paint and an incremental paint of a
> smaller region.

I recall looking into something like this.  IIRC a focus-in event was causing the full repaint.  This only happened during the first test run, so I assumed it wasn't an issue for most tests, but I guess this doesn't explain comment 3 where the full reftest suite was run.
(Reporter)

Comment 9

6 years ago
The full reftest suite wasn't run -- the attached patch replaces all of reftest.list with a dead-simple manifest w/ two reftests.

So -- not a full test-run, but still more than 1 test (2, to be precise) that passed when they should've failed.
(Reporter)

Comment 10

6 years ago
Oh, wait -- re-reading comment 3, looks like just the first test incorrectly passed -- the second was fine.

So, it looks like I was hitting exactly what you described.
Summary: MozReftestInvalidate doesn't reliably test invalidation. (Full-window-repaint happens after it fires) → MozReftestInvalidate doesn't reliably test invalidation in first reftest (Full-window-repaint due to focus-in after MozReftestInvalidate)
(Reporter)

Comment 11

6 years ago
Unassigning, because this isn't as severe as I thought -- it only affects the first reftest in a reftest run, per comment 8 / comment 10 -- so it's lower-priority than other things that I'm working on at the moment, and I probably won't get to it anytime soon.
Assignee: dholbert → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.