Closed Bug 578589 Opened 10 years ago Closed 9 years ago

Intermittent failure in toolkit/content/tests/widgets/test_toolbar.xul | | checkSet(76) correct ID /^spacer\d+/ for toolbar tb-test; got: p12 followed by two more checkSet(76) failures

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: ehsan, Assigned: mak)

References

Details

(Keywords: intermittent-failure, Whiteboard: [time-bomb][relevant comments: 262,263,264,278,286])

Attachments

(1 file)

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1279060632.1279061242.30409.gz
WINNT 5.2 mozilla-central opt test mochitests-5/5 on 2010/07/13 15:37:12

12786 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_toolbar.xul | checkSet(76) correct ID /^spacer\d+/ for toolbar tb-test; got: p12
12787 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_toolbar.xul | checkSet(76) correct ID p12 for toolbar tb-test****/^separator\d+/,/^spacer\d+/,/^separator\d+/,p11,/^spring\d+/,/^spacer\d+/,p12,/^spring\d+/,6, - "spring12790611592626" should equal "p12"
12788 INFO TEST-PASS | /tests/toolkit/content/tests/widgets/test_toolbar.xul | checkSet(76) extra fixed items for tb-test - 0 should equal 0
12789 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_toolbar.xul | checkSet(76) correct number of children for tb-test - 7 should equal 8
This failure seems to be happening on Mac every or almost every time since October 13. Before then, it was Windows only.
Most likely trigger for the explosion of instances is bug 553098 making it all faster (though I don't think I've ever seen anything where Windows was the only thing _fast_ enough to hit something before).
OS: Windows Server 2003 → All
Hardware: x86 → All
I think I have a fix for this.

What happens is that in toolbar.xml we do:

  case "separator":
  case "spring":
  case "spacer":
    newItem = document.createElementNS(XUL_NS, "toolbar" + aId);
    newItem.id = aId + Date.now() + this.childNodes.length;

Date.now() does not have a really high resolution, on Windows for example we know timers have about 16ms resolution, and on VMs this is often worse. So a lot of sub-tests will run with the same timestamp that I will call TIMESTAMP.

childNodes.length is there to solve this issue, but it changes along the test. not only increasing but also decreasing.

So in test 73 we have 2 nodes: p11, p12
in test 74 and 75 we have 5 nodes: separator,separator,p11,spring,spacer

what happens is that when the last spacer is created childNodes is 5 (because p12 will be removed later, see http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/toolbar.xml#244) so we create spacerTIMESTAMP5

in test 76 we have 8 nodes: separator,spacer,separator,p11,spring,spacer,p12,spring

notice that spacer is at position 6. Since timers suck when toolbar creates spacer it will create spacerTIMESTAMP5... WTF, this is the same we already inserted in position 2!

Solution is pretty simple, don't rely on childNodes.length, use a field instead!
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Whiteboard: [orange] → [orange][time-bomb]
Attached patch patch v1.0Splinter Review
Attachment #486338 - Flags: review?(enndeakin)
Attachment #486338 - Flags: review?(enndeakin) → review+
Comment on attachment 486338 [details] [diff] [review]
patch v1.0

a=beltzner

I'm fine with this landing through the beta7 freeze if it gets a clean run through tryserver. Feels like a good win to crack down on some orange.
Attachment #486338 - Flags: approval2.0+
FWIW run on tryserver was fine, apart usual and known oranges:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=47430d50b07b

I was asked if this could still be a problem for people who sets the clock in the past/future for testing. I think it should not be, the possibility of collision is bound to the event that the user creates the same exact type of toolbar element in the same exact millisecond with the same exact creation count.
This possibility was already small enough before for common users, just an issue in tests. Persisting a monotonic counter could be a greater cost compared to the gain.
http://hg.mozilla.org/mozilla-central/rev/eefdc0e5e208
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [orange][time-bomb] → [orange][time-bomb][relevant comments: 262,263,264,278,286]
Target Milestone: --- → mozilla2.0b7
comment 287 is before the fix push, fwiw
Whiteboard: [orange][time-bomb][relevant comments: 262,263,264,278,286] → [time-bomb][relevant comments: 262,263,264,278,286]
You need to log in before you can comment on or make changes to this bug.