Closed Bug 1070923 Opened 11 years ago Closed 11 years ago

Intermittent test_bug451286.xul | Highlighting in iframe correctly removed - expected PASS

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Type:
defect
Priority:
Not set
Severity:
normal
Points:
5

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35
Tracking Flags:
Tracking Status
firefox33 --- unaffected
firefox34 --- unaffected
firefox35 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: cbook, Assigned: tomasz)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

intermittent-find.patch
6.42 KB, patch
tomasz
: review+
Details | Diff | Splinter Review
Rev4 MacOSX Snow Leopard 10.6 mozilla-inbound debug test mochitest-other on 2014-09-22 02:23:00 PDT for push 3161ad541392 slave: t-snow-r4-0106 https://tbpl.mozilla.org/php/getParsedLog.php?id=48571297&tree=Mozilla-Inbound 1727 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_bug451286.xul | Highlighting in iframe correctly removed - expected PASS
Tomasz, I suspect this intermittent failure is fallout from bug 429732. Can you take a look?
Flags: needinfo?(tkolodziejski)
OS: Mac OS X → All
Hardware: x86 → All
This intermittent is timeout related. I can reliably reproduce it by changing: gFindBar.addEventListener('transitionend', part2); to setTimeout(part2, 0). It was introduced by my changes as they made highlight async. My plan is to add method that will inform listeners that highlight has finished its job.
Flags: needinfo?(tkolodziejski)
Depends on: 429732
Assignee: nobody → tkolodziejski
Points: --- → 5
Attached patch intermittent-find.patch (obsolete) — — Splinter Review
The patch should be fixing the issue. Why was it happening: 1. The test had a couple of functions 2. Those functions are taking screenshots 3. Each function was waiting for the findbar to get closed before the next function took the screenshot (the reason for that is that with findbar opened the size of the screenshots were different because findbar is taking up space. I added a comment about that to the test file so the next person to read it does not have to figure it out...) 4. Running the functions using this listener created race condition: it sometimes happens (as we see in this bug) that the transitionend event was fired before the highlight was finished and so the solution. Also I am deeply surprised that the result of the code: Task.spawn(function*() {console.log('a');}); console.log('b'); is showing a b and not the opposite. This means that notifyHighlightFinished is firing asynchronously only because above it there is a yield promise (yield this._highlight(aHighlight, aWord, null)). It is very dangerous to create functions that are sometimes async and in this case it just happened to be ok. I pushed it to try: https://tbpl.mozilla.org/?tree=Try&rev=68aa6c242df1. When it finishes I may try to retrigger this particular test (suite?) a couple of times. And what about beenHere1? adw pointed out that it (probably) was already there: https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&longdesc=test_eventemitter_basic&longdesc_type=substring&list_id=11222025.
Attachment #8493421 - Flags: review?(mdeboer)
Comment on attachment 8493421 [details] [diff] [review] intermittent-find.patch Review of attachment 8493421 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Tomasz! This does indeed seem to fix it :) ::: toolkit/content/tests/chrome/bug451286_window.xul @@ +62,5 @@ > + // on the new snapshot. > + function closeFindbarAndWait() { > + return new Promise((resolve) => { > + gFindBar.close(); > + gFindBar.addEventListener('transitionend', function cont (aEvent) { nits: please use double quotes for strings. This applies to the entire changeset. Also, the additional space after `cont<space>(aEvent) {` is not necessary. I know you didn't write this code, but since you're touching it now you might as well correct it! :)
Attachment #8493421 - Flags: review?(mdeboer) → review+
Attachment #8493421 - Attachment is obsolete: true
Attachment #8496102 - Flags: review+
I addressed your comments. Marking this as checkin-needed. Let's hope it fixes the issue.
Keywords: checkin-needed
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/58a26fd7e4de
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: