Closed Bug 471365 Opened 16 years ago Closed 16 years ago

Make reftests able to catch invalidation bugs

Categories

(Testing :: Reftest, defect)

Product:
Testing
Component:
Reftest
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(1 file)

fix
21.39 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
I'm tired of not being able to test invalidation bugs and we don't need to wait for compositor to do it. I think we can do it just by having reftest.js listen for MozAfterPaint and do a canvas.drawWindow after each one, drawing just the MozAfterPaint's invalid area to the canvas.

The only tricky bit is how to actually write reftests that reliably test invalidation. They need to make sure they actually trigger the invalidation *after* reftest.js has taken a snapshot of the window. They also have to make sure that there are no pending invalidations that will invalidate the whole window anyway.

Painting is unsuppressed after onload fires, which normally invalidates the entire content window, so we need to make sure test invalidation is triggered after that invalidation has been flushed.

Here's a plan:
1) Set 'reftest-wait' class on invalidation reftest root element
2) When reftest.js OnDocumentLoad happens, when reftest-wait is set it should queue a setTimeout(0) which does the following, synchronously:
2a) Call nsIDOMWindowUtils::ProcessUpdates to flush out pending invalidates
2b) Snapshot the whole window into the canvas
2c) Add a MozAfterPaint listener to copy future invalidate/paint areas to the
target canvas. Since there was not such listener already, invalidation that happened before this point should not even be queued and will not be forwarded to the new listener.
2d) Dispatch a custom event, say MozReftestInvalidate, at the content window
2e) Content window should have registered a MozReftestInvalidate event handler which will do the actual invalidation-triggering action and (either immediately or eventually) removes the reftest-wait class
3) When reftest-wait is removed, reftest.js does the following, synchronously:
3a) Do a complete FlushPendingNotifications
3b) Call a new method nsIDOMWindowUtils::HasPendingMozAfterPaint to see if there is a MozAfterPaint event pending. If there isn't, end the test now.
Otherwise, set a flag so that after the next MozAfterPaint event is handled,
with the canvas updated, we then end the test.

So a test would look like this:

<html class="reftest-wait">
<body>
<script>
function f() {
  document.body.style.background = 'yellow';
  document.documentElement.removeAttribute('class');
}
window.addEventListener("MozReftestInvalidate", f, false);
</script>
</body>
</html>
Once we have this sorted out, we could do some fun random testing where you take some HTML, add a fixed-pos opaque element that covers some region of the HTML, and on MozReftestInvalidate remove that element. Compare the result to the test running alone. That should catch all kinds of invalidation and overflow area bugs.
Adding bug 451332 as dep, so we can track its deps.  I suppose we could copy the dep list and dup it here, but that's more bugmail...
Blocks: 451332
Ha, I've already found one bug with this! layout/reftests/svg/dynamic-filter-contents-01.svg actually fails to invalidate correctly.
Bug 471629, bug 471630, and bug 471631 filed based on failures in existing reftests that I found with the new framework.
Jonathan, Robert --- I know it was your intention to make the SVG reftests cross-browser. However, the dynamic SVG reftests mostly have code like onload="setTimeout(doTest, 500)" to try to wait for all repainting to complete before triggering a dynamic change. With the infrastructure I'm going to add in this bug, we'll have a MozReftestInvalidate event that will fire as soon as repainting is complete and we're OK to test a dynamic change. But that is of course not cross-browser. What do you think we should do here? I would really like to get rid of the setTimeouts because they slow down testing and mean that the tests are unreliable ... for these dynamic SVG tests, that means that on a slow machine the test may report false negatives --- which aren't as bad as false positives, but still...
Attached patch fix — — Splinter Review 
-- Refactor nsDOMWindowUtils to share some code to get the prescontext
-- Implement IsMozAfterPaintPending
-- Mark a few reftests as failing/random because the new test code exposes invalidation bugs in them
-- Add a reftest-sanity test to show that the MozReftestInvalidate event fires (I've also tried using MozReftestInvalidate in dynamic-filter-contents-02.svg, and it correctly exposes the invalidation bug there)
-- reftest.js changes to track invalidations after OnDocumentLoaded, fire MozReftestInvalidate, etc
Attachment #354916 - Flags: review?
Attachment #354916 - Flags: review? → review?(dbaron)
So is the idea here that we want to run both test and reference using a canvas built up by successive invalidates?  Might we not want to run both the tests and the references in both modes, or is that overkill?

Or maybe the normal way of invoking reftests should be to run both test and reference in the new mode, but there should be an alternate way of running reftests (including the "load" tests that are mostly in crashtests) in a mode where we compare the invalidate-built canvas aganst a final drawWindow?
(In reply to comment #7)
> So is the idea here that we want to run both test and reference using a canvas
> built up by successive invalidates?

That's what the patch does, yes.

> Might we not want to run both the tests
> and the references in both modes, or is that overkill?

That's quite easy to do, but it makes things a bit more complicated because it means a test failure could be in one or more "modes". It could get confusing with != tests ... in theory you'd want to test that incremental-test == nonincremental-test, incremental-reference == nonincremental-reference, and incremental-test != nonincremental-reference.

> Or maybe the normal way of invoking reftests should be to run both test and
> reference in the new mode, but there should be an alternate way of running
> reftests (including the "load" tests that are mostly in crashtests) in a mode
> where we compare the invalidate-built canvas aganst a final drawWindow?

Right now, only one reference uses reftest-wait --- layout/reftests/bugs/406073-1-ref.html --- so it doesn't really matter what mode we use for references.

A simple thing we could do is check that the incremental and nonincremental versions of a "load" reftest are equal. How does that sound? It means that there's still only one canvas check per test, which is good because a test failure remains unambiguous without having to add extra information to the error messages and without confusion around != tests.
(In reply to comment #5)
> Jonathan, Robert --- I know it was your intention to make the SVG reftests
> cross-browser. However, the dynamic SVG reftests mostly have code like
> onload="setTimeout(doTest, 500)" to try to wait for all repainting to complete
> before triggering a dynamic change. With the infrastructure I'm going to add in
> this bug, we'll have a MozReftestInvalidate event that will fire as soon as
> repainting is complete and we're OK to test a dynamic change. But that is of
> course not cross-browser. What do you think we should do here? I would really
> like to get rid of the setTimeouts because they slow down testing and mean that
> the tests are unreliable ... for these dynamic SVG tests, that means that on a
> slow machine the test may report false negatives --- which aren't as bad as
> false positives, but still...

Can't we do both; have a cross-browser timeout that is only set up if the test is not run on gecko?
Yeah, I guess we could have something like

<script src="svg-helpers.js"></script>
<script>
function doInvalidate() {
  document.getElementId("x").setAttribute("id", "y");
  ... etc ...
}
</script>

where svg-helpers.js contains code that listens for MozReftestInvalidate if we're on Gecko and a setTimeout otherwise.
I'll file a separate bug to fix the SVG tests.
(In reply to comment #8)
> A simple thing we could do is check that the incremental and nonincremental
> versions of a "load" reftest are equal. How does that sound? It means that
> there's still only one canvas check per test, which is good because a test
> failure remains unambiguous without having to add extra information to the
> error messages and without confusion around != tests.

I think I'd prefer to keep load testing a strict subset of what == tests.  I'd like to think about this a little more.  How about starting with what's here (without the modification you describe) and then we'll think about this more after this lands?
So after reading reftest.js, it seems like this is only testing invalidates that happen after document load completes, not those that happen before it.  It seems like some invalidation bugs are related to invalidates that happen during load (e.g., images loading and causing reflow).  Does this test those somehow?
(In reply to comment #12)
> I think I'd prefer to keep load testing a strict subset of what == tests.  I'd
> like to think about this a little more.  How about starting with what's here
> (without the modification you describe) and then we'll think about this more
> after this lands?

OK. (Another option would be to have a new keyword like "load" which does the incremental vs nonincremental test.)

(In reply to comment #13)
> So after reading reftest.js, it seems like this is only testing invalidates
> that happen after document load completes, not those that happen before it.

Correct.

> It seems like some invalidation bugs are related to invalidates that happen
> during load (e.g., images loading and causing reflow).  Does this test those
> somehow?

No. But in most cases those could be tested by dynamically inserting the content when the load event fires. Writing reliable tests that depend on invalidation happening before onload would be hard --- how would you reliably exit paint suppression?
Have you tested the interaction of invalidation reftests with zoom reftests?

Could you add a comment explaining when gCurrentCanvas is null and when it's not?  (I haven't figured that out yet, although maybe I will shortly.)

Any reason you chose to use so many nested functions?  I'd probably have made most or all of them toplevel.  (It seems odd for some of the event listeners that aren't document load to be inside DocumentLoaded but some not to be.)
Hmm.  I'm a little unhappy about calling DocumentLoaded(... ,0) to force the final pass.  Could that be factored out into a separate function so that we don't call DocumentLoaded at any time other than document load?

Also, perhaps StartWaiting and FinishWaiting should be named ...WaitingForInvalidates?
(In reply to comment #16)
> Hmm.  I'm a little unhappy about calling DocumentLoaded(... ,0) to force the
> final pass.  Could that be factored out into a separate function so that we
> don't call DocumentLoaded at any time other than document load?

Er, never mind this one, I guess we already did that.
(In reply to comment #15)
> Have you tested the interaction of invalidation reftests with zoom reftests?

No. I don't expect it to work well for non-integer scales, we'd need to round the invalid rect edges to "device coordinates" at least. What do you want me to do about that?

> Could you add a comment explaining when gCurrentCanvas is null and when it's
> not?  (I haven't figured that out yet, although maybe I will shortly.)

It's non-null between InitCurrentCanvasWithSnapshot and the next DocumentLoaded.

> Any reason you chose to use so many nested functions?  I'd probably have made
> most or all of them toplevel.  (It seems odd for some of the event listeners
> that aren't document load to be inside DocumentLoaded but some not to be.)

That would mean moving a few more variables to be global. I can do that if you like.
(In reply to comment #16)
> Also, perhaps StartWaiting and FinishWaiting should be named
> ...WaitingForInvalidates?

Sure OK.
(In reply to comment #18)
> That would mean moving a few more variables to be global. I can do that if you
> like.

Which variables?
stopAfterPaintReceived at least. Maybe utils and currentDoc too.
Comment on attachment 354916 [details] [diff] [review]
fix

OK, I guess the nesting is ok as-is, so r=dbaron.

As far as the zoom goes, I think you should add the needed snap-round-out code and probably add a test for it.  We don't want reftest-wait + reftest-zoom reftests that weren't even intending to test invalidation to fail because of this.
Attachment #354916 - Flags: review?(dbaron) → review+
Could we add comments to the reftest readme explaining how to use this stuff, if the reftest needs to do anything special to make use of it?
(In reply to comment #19)
> (In reply to comment #16)
> > Also, perhaps StartWaiting and FinishWaiting should be named
> > ...WaitingForInvalidates?
> 
> Sure OK.

Well, we're not really waiting for invalidates. We're waiting for reftest-wait to be removed, which may have nothing to do with invalidation. I'll change to StartWaitingForTestEnd/EndWaitingForTestEnd.
Pushed 502ad51d0af3 with requested changes.
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Version: unspecified → Trunk
This bug won't land on m-1.9.1, right?
Target Milestone: --- → mozilla1.9.2a1
Depends on: 474472
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: