Closed Bug 555392 Opened 14 years ago Closed 12 years ago

Intermittent failure in layout/reftests/svg/smil/container/deferred-tree-1.xhtml

Categories

(Core :: SVG, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16
Tracking Flags:
Tracking Status
firefox15 --- fixed

People

(Reporter: philor, Assigned: birtles)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

reftest log lines (with snapshots)
20.84 KB, text/plain
Details
Proposed fix v1a
5.19 KB, patch
dholbert
: review+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review 
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1269641655.1269644247.24549.gz
OS X 10.5.2 mozilla-central debug test reftest on 2010/03/26 15:14:15
s: moz2-darwin9-slave42

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-debug-unittest-reftest/build/reftest/tests/layout/reftests/svg/smil/container/deferred-tree-1.xhtml
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270163844.1270166416.8956.gz
OS X 10.5.2 mozilla-central debug test reftest on 2010/04/01 16:17:24
s: moz2-darwin9-slave39
Here's the reftest log from today's failure, suitable for pasting into reftest-analyzer.xhtml

In both logs posted here, the testcase has the yellow oval positioned up high, while the reference case has it further down.  The failures aren't exactly the same (vertical position is shifted slightly) but they're pretty close.

Looking at the source of the testcase...
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/smil/container/deferred-tree-1.xhtml
... it looks like it uses a setTimeout(150) to schedule the reftest-snapshot.  We need to fix this test to fix the test so it doesn't need to rely on that timeout.
This reftest and deferred-tree-2* are apparently running into some few-px-difference failures on the Cedar branch, too -- e.g.:
http://tinderbox.mozilla.org/showlog.cgi?log=Birch/1284407086.1284408730.26415.gz#err11

Those failures might be forms of bug 470868 (since they're 16px diffs) or forms of this bug here. (Or this bug and bug 470868 might really be the same underlying problem.)

Since stuff from Cedar is moving to mozilla-central Real Soon Now, I'm going to fix this test to not use setTimeout, to see if that fixes the Cedar issues.  (It should fix the not-seen-for-a-while problems here, too, per comment 2)
Assignee: nobody → dholbert
Er, I meant "Birch", not "Cedar"

However, I've now noticed that removing setTimeout won't fix those issues, because those the deferred-tree-2* tests don't use setTimeout at all, so there's nothing setTimeout-related to fix there.

(Also, the setTimeout can't be removed from deferred-tree-1.xhtml as easily as I'd thought -- my instinct was to replace it with a setCurrentTime, but that testcase has a comment saying it's explicitly *not* using setCurrentTime to make sure animation starts on its own.)

In any case, the Birch issues from comment 6 are bug 470868, not this bug here. Taking the discussion there.
Assignee: dholbert → nobody
Running something through try for this now.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Attached patch Proposed fix v1a — — Splinter Review 
Here's an attempt to fix this. I've just changed the approach of the test to wait for an end event and finish the test then and timeout if the event never arrives. I've also simplified the test somewhat.

An initial run on try looks fine: https://tbpl.mozilla.org/?tree=Try&rev=2d2326caa9af

It's usually Android (native) R3 that fails so I'll try to trigger that once or twice more.
Attachment #639538 - Flags: review?(dholbert)
Attachment #639538 - Flags: review?(dholbert) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Thank you Brian! :-D
(In reply to Ed Morley [:edmorley] from comment #211)
> Thank you Brian! :-D

Not at all, sorry it took me so long to get to this.
Comment on attachment 639538 [details] [diff] [review]
Proposed fix v1a

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown
User impact if declined: Higher rate of [orange]
Testing completed (on m-c, etc.): m-c, aurora
Risk to taking this patch (and alternatives if risky): N/A, test-only fix.
String or UUID changes made by this patch: None
Attachment #639538 - Flags: approval-mozilla-beta?
Target Milestone: --- → mozilla16
(In reply to Ed Morley [:edmorley] from comment #214)
> Risk to taking this patch (and alternatives if risky): N/A, test-only fix.

In the pre-rapid-release days, test-only tweaks were allowed to land on branches without approval (as long as they're sane/tested), since they're NPODB.

I think that might still be the case, but I'm not 100% sure.
Comment on attachment 639538 [details] [diff] [review]
Proposed fix v1a

test-only, NPOTB - I'm with dholbert here, if a test-only fix (that doesn't touch the build) needs to land on branches, I'm OK with it being landed with a=NPOTB.
Attachment #639538 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Lukas Blakk [:lsblakk] from comment #219)
> test-only, NPOTB - I'm with dholbert here, if a test-only fix (that doesn't
> touch the build) needs to land on branches, I'm OK with it being landed with
> a=NPOTB.

Sounds good to me :-)
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: