Closed
Bug 547801
Opened 15 years ago
Closed 12 years ago
REFTEST TEST-UNEXPECTED-FAIL | ...layout/reftests/svg/smil/sort/sort-additive-1.svg
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: zpao, Unassigned)
References
()
Details
(Keywords: intermittent-failure)
Attachments
(4 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
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
Updated•15 years ago
|
Whiteboard: [orange]
Comment 1•15 years ago
|
||
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.
Comment 2•15 years ago
|
||
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.
Updated•15 years ago
|
Comment 3•15 years ago
|
||
Taking -- I'll try to investigate this further in the next day or two.
Assignee: nobody → dholbert
Comment 4•15 years ago
|
||
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.
Comment 5•15 years ago
|
||
I filed bug 548132 on the bust timeout ordering, since that seems bad all the same.
Comment 6•15 years ago
|
||
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.
Comment 7•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #428833 -
Attachment is patch: true
Attachment #428833 -
Attachment mime type: application/octet-stream → text/plain
Updated•15 years ago
|
Assignee: dholbert → jwatt
Comment 8•15 years ago
|
||
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. :)
Updated•15 years ago
|
Attachment #428833 -
Attachment description: catch setTimeout bad order → debugging patch: catch setTimeout bad order
Updated•15 years ago
|
Attachment #428833 -
Flags: review+
Comment 9•15 years ago
|
||
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
Updated•15 years ago
|
Whiteboard: [orange] → [orange][debugging aid landed]
Updated•15 years ago
|
Attachment #428833 -
Attachment description: debugging patch: catch setTimeout bad order → debugging patch: catch setTimeout bad order [landed]
Comment 10•15 years ago
|
||
If this fails again, hopefully the push in comment 9 will give us more information about what's going on...
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•14 years ago
|
Assignee: jwatt → nobody
Comment 13•13 years ago
|
||
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 14•13 years ago
|
||
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-
Comment 15•13 years ago
|
||
Fixed review comments
Attachment #599390 -
Attachment is obsolete: true
Attachment #601818 -
Flags: review?(dholbert)
Updated•13 years ago
|
Attachment #601818 -
Flags: review?(dholbert) → review+
Comment 16•13 years ago
|
||
(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.)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 22•13 years ago
|
||
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 25•13 years ago
|
||
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.
Comment 26•13 years ago
|
||
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.)
Comment 27•13 years ago
|
||
Oh! Great point. I didn't initially notice that the failures were android-specific.
Agreed.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 32•13 years ago
|
||
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]
Comment 33•13 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #32)
> I'm taking comment 26 as a rubber-stamp
Thanks!
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 36•13 years ago
|
||
(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
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 51•12 years ago
|
||
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.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 66•12 years ago
|
||
bug 781362 seems to have fixed this.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
Keywords: intermittent-failure
Assignee | ||
Updated•12 years ago
|
Whiteboard: [orange]
You need to log in
before you can comment on or make changes to this bug.
Description
•