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

Status: NEW

(Testing :: Reftest, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 2

[Daniel Holbert [:dholbert]]

(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.)

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

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.)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Daniel Holbert [:dholbert]]

(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.

Comment 7

[Daniel Holbert [:dholbert]]

s/days/weeks/

Comment 8

[Karl Tomlinson (:karlt)]

(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.

Comment 9

[Daniel Holbert [:dholbert]]

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.

Comment 10

[Daniel Holbert [:dholbert]]

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.

Comment 11

[Daniel Holbert [:dholbert]]

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.