Closed Bug 547801 Opened 14 years ago Closed 12 years ago

REFTEST TEST-UNEXPECTED-FAIL | ...layout/reftests/svg/smil/sort/sort-additive-1.svg

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: zpao, Unassigned)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(4 files, 1 obsolete file)

reftest log lines (with snapshots)
18.72 KB, text/plain
Details
debugging patch: catch setTimeout bad order [landed]
1.80 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
trivial fix: remove setTimeouts
1.03 KB, patch
Details | Diff | Splinter Review
Use MozReftestInvalidate v2
1.85 KB, patch
dholbert
: review+
Details | Diff | Splinter Review 
First seen:
OS X 10.5.2 mozilla-central opt test reftest on 2010/02/22 10:11:51
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1266862311.1266863429.11456.gz

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-opt-unittest-reftest/build/reftest/tests/layout/reftests/svg/smil/sort/sort-additive-1.svg |
...
REFTEST number of differing pixels: 2040
Whiteboard: [orange]
Here's the reftest log (with snapshots).  reftest-analyzer.xhtml shows that the failure is that the 7th blue box (from the top) ends up on the 3rd line instead of the 2nd line.
This test includes
  setTimeout('setTimeAndSnapshot(3.1, true)',20);

That setTimeout seems shady -- if it's needed for this test to pass, it's probably the source of this (low-frequency) failure.
Taking -- I'll try to investigate this further in the next day or two.
Assignee: nobody → dholbert
Reftest has:
>    function swapAnimations() {
>      setTimeout('shuffleAnimations()',10);
>      setTimeout('setTimeAndSnapshot(3.1, true)',20);
>    }

Clearly, it expects shuffleAnimations() to run before setTimeAndSnapshot().  If I reverse the order by swapping the timeout amounts, I get a failure exactly like the one reported here.  So, the setTimeout ordering isn't entirely reliable here.

Turns out we don't need the setTimeouts in the first place, though -- we still pass the test if I remove them.  I think that's what we should do.
I filed bug 548132 on the bust timeout ordering, since that seems bad all the same.
Actually it seems unlikely that the events are firing out of order (see bug 548132 comment 4), so your conclusion in comment 4 is probably mistaken Daniel.
Maybe we should commit something like this so that the next time we get a failure we know for sure whether it's setTimeout misordering or not.
Attachment #428833 - Attachment is patch: true
Attachment #428833 - Attachment mime type: application/octet-stream → text/plain
Assignee: dholbert → jwatt
Here's (what should be) a trivial fix for this, doing what I suggest at the end of comment 4. (just remove the setTimeouts)

I agree that we should use jwatt's debugging-patch for a while first, though, to get a little more information about what's going on here if we ever hit this same failure again. :)
Attachment #428833 - Attachment description: catch setTimeout bad order → debugging patch: catch setTimeout bad order
Attachment #428833 - Flags: review+
Pushed jwatt's patch, with this typo fix:
  s/document.documentElement.createElementNS/document.createElementNS/
I verified that it passes locally, and that it draws the (new) giant red line if I forcibly reverse the setTimeout order.

http://hg.mozilla.org/mozilla-central/rev/210e12a88a68
Whiteboard: [orange] → [orange][debugging aid landed]
Attachment #428833 - Attachment description: debugging patch: catch setTimeout bad order → debugging patch: catch setTimeout bad order [landed]
If this fails again, hopefully the push in comment 9 will give us more information about what's going on...
Assignee: jwatt → nobody
Attached patch Use MozReftestInvalidate (obsolete) — — Splinter Review 
This test is failing because of the way that the reftest harness detects paints, and updates the snapshot.

The reftest harness will only take a new snapshot if it detects a paint is pending after calling the MozReftestInvalidate event.

Since the invalidation is instead called from a timer, we can end up in the situation where it invalidates, and then does a normal window paint (clearing the pending paint flag), before the reftest harness checks again.

In this case the reftest harness thinks no paints have happened, and just uses the initial (unmodified) snapshot.


Triggering the invalidation/removal of reftest-test from the MozReftestInvalidate event should prevent this.
Attachment #599390 - Flags: review?(dholbert)
Comment on attachment 599390 [details] [diff] [review]
Use MozReftestInvalidate

As noted in IRC:
 - patch is mostly unrelated stuff right now. :)
 - I think you want s/doTest/swapAnimations/
 - you also want to remove onload=swapAnimations
Attachment #599390 - Flags: review?(dholbert) → review-
Fixed review comments
Attachment #599390 - Attachment is obsolete: true
Attachment #601818 - Flags: review?(dholbert)
Attachment #601818 - Flags: review?(dholbert) → review+
(Note: When attachment 601818 [details] [diff] [review] lands, we can remove the "[debugging aid landed]" whiteboard status, since attachment 601818 [details] [diff] [review] removes that debugging code.)
It looks like the MozReftestInvalidate tweak has made this worse instead of better. :(  4 oranges already today, and it's only 10:30 AM.

We should probably back out that change.
Or if it's the right thing to do, maybe we should mark the test as random on Android? I wonder if the assumptions the code that dispatches MozReftestInvalidate makes are not correct on Android. (Since it paints differently.)
Oh! Great point. I didn't initially notice that the failures were android-specific.

Agreed.
I'm taking comment 26 as a rubber-stamp on the idea of marking the reftest as random-if(Android), and I landed a patch to do that, to stop the orange.
  https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb0cfaa1939
Whiteboard: [orange][debugging aid landed] → [orange]
(In reply to Daniel Holbert [:dholbert] from comment #32)
> I'm taking comment 26 as a rubber-stamp

Thanks!
(In reply to Daniel Holbert [:dholbert] from comment #32)
> I'm taking comment 26 as a rubber-stamp on the idea of marking the reftest
> as random-if(Android), and I landed a patch to do that, to stop the orange.
>   https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb0cfaa1939

https://hg.mozilla.org/mozilla-central/rev/ffb0cfaa1939
FWIW: These latest "(waiting for MozAfterPaint)" failures are instances of bug 758505 comment 5 -- DLBI prevents us from ever firing MozReftestInvalidate in this testcase.  There are animations going right away, so we never reach a "stable" state (at least, not until all animations run to completion, and we definitely don't want to wait that long).

We probably need to fix the test by giving each animation begin="100s" or something, so that we'll start out in stable state, and then adjust any seeking as necessary.
Depends on: 781362
bug 781362 seems to have fixed this.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: