Closed Bug 1187008 Opened 9 years ago Closed 9 years ago

Intermittent browser_bug537013.js | Highlight button reset!

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 42
Tracking Flags:
Tracking Status
firefox40 --- unaffected
firefox41 --- unaffected
firefox42 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: RyanVM, Assigned: unusualtears)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

Update for checkin, use arrow function for correctness
2.11 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review 
Mike, this started around the time that bug 1167601 landed. Can you please take a look?

10:08:10 INFO - 154 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | Set the field correctly!
10:08:10 INFO - 155 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | 'TabFindInitialized' event properly dispatched!
10:08:10 INFO - 156 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | Second tab doesn't show find bar!
10:08:10 INFO - 157 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | Second tab kept old find value for new initialization!
10:08:10 INFO - 158 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | Set the field correctly!
10:08:10 INFO - 159 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | First tab shows find bar!
10:08:10 INFO - 160 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | First tab persists find value!
10:08:10 INFO - 161 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | Highlight button state persists!
10:08:10 INFO - 162 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug537013.js | Highlight button reset! -
10:08:10 INFO - Stack trace:
10:08:10 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug537013.js:continueTests2:81
10:08:10 INFO - null:null:0
Flags: needinfo?(mconley)
I'm not yet convinced that bug 1167601 is at fault here... the test doesn't seem to involve newtab at all, and none of the changes should have affected findbar (which the test exercises).

I've done a bunch of retriggers on the push that landed this patch. Let's see if it turns up - but I have my doubts.
Still no hits on that push. I've triggered a bunch more, but I'm about to plead innocent on this one.
RyanVM: do you think the number of retriggers of bc2 for Linux64 debug[1] is enough to exonerate bug 1167601?

[1]: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=ee371e7b0af0
Flags: needinfo?(mconley) → needinfo?(ryanvm)
Seems reasonable.

Adam, I see that you originally wrote this test. Would you by chance have time to look into what might be failing here?
Flags: needinfo?(ryanvm) → needinfo?(unusualtears)
The timing has changed, but hard to say why.

Expected:  

1. Tab reloads  
2. Tab's progress listener's |onLocationChange| event unchecks the highlight button (tabbrowser.xml:~794)  
3. Test's |DOMContentLoaded| event listener gets called  
4. Test of the highlight button's checked state passes  

Intermittent failure case:  

1. Tab reloads  
2. Test's |DOMContentLoaded| event listener gets called  
3. Test of the highlight button's checked state fails  
4. [Assumed] Tab's progress listener's |onLocationChange| event unchecks the highlight button  

Pushed a patch to try that uses |waitForCondition| to outwait the race:  

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1729f864eb6

Will keep retriggering the M(bc2) tests on what appears the worst offender (Linux x64 debug) for awhile to see if the failure recurs.
Flags: needinfo?(unusualtears)
My hero, thanks.
The main change, as mentioned, is to |waitForCondition| before the offending test. The try results showed no sign of recurrence.

I also added encoding info to the test cases, just to make the character encoding detection code happy.
Assignee: nobody → unusualtears
Status: NEW → ASSIGNED
Attachment #8640121 - Flags: review?(mconley)
Comment on attachment 8640121 [details] [diff] [review]
Fix intermittent failure of findbar highlight button test

Review of attachment 8640121 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the change. Thanks!

::: browser/base/content/test/general/browser_bug537013.js
@@ +78,5 @@
>  }
>  
>  function continueTests2() {
>    gBrowser.removeEventListener("DOMContentLoaded", continueTests2, true);
> +  waitForCondition(function() !gFindBar.getElement("highlight").checked,

I'm pretty sure this style of function declaration is no longer one we want to use - can you use:

waitForCondition(() => !gFindBar.getElement("highlight").checked,
                 continueTests3,
                 "Highlight never reset!");

instead?
Attachment #8640121 - Flags: review?(mconley) → review+
Thanks, Mike!
Attachment #8640121 - Attachment is obsolete: true
Attachment #8640134 - Flags: checkin?
Keywords: checkin-needed
Attachment #8640134 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/5de665a51c3e

Thanks for jumping on this, Adam!
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: