Closed Bug 1397728 Opened 4 years ago Closed 4 years ago

Intermittent browser/components/search/test/browser_searchbar_openpopup.js | Should show the full popup - Didn't expect true, but got it

Categories

(Firefox :: Search, defect, P2)

Product:
Firefox
Component:
Search
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: standard8)

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed])

Attachments

(1 file)

Bug 1397728 - Use waitForCondition to avoid intermittent failures in browser_searchbar_openpopup.js.
59 bytes, text/x-review-board-request
florian
: review+
Details 
Filed by: archaeopteryx [at] coole-files.de

https://treeherder.mozilla.org/logviewer.html#?job_id=129201771&repo=autoland

https://queue.taskcluster.net/v1/task/XHLDw1eMQZGwSkYnRC8jTw/runs/0/artifacts/public/test_info/browser-chrome-chunked_errorsummary.log

11:38:26     INFO -  351 INFO Entering test bound drop_opens_popup
11:38:26     INFO -  Buffered messages finished
11:38:26    ERROR -  352 INFO TEST-UNEXPECTED-FAIL | browser/components/search/test/browser_searchbar_openpopup.js | Should show the full popup - Didn't expect true, but got it
11:38:26     INFO -  Stack trace:
11:38:26     INFO -  chrome://mochikit/content/browser-test.js:test_isnot:1011
11:38:26     INFO -  chrome://mochitests/content/browser/browser/components/search/test/browser_searchbar_openpopup.js:drop_opens_popup:487
11:38:26     INFO -  353 INFO TEST-PASS | browser/components/search/test/browser_searchbar_openpopup.js | Should have focused the search bar -
11:38:26     INFO -  354 INFO Leaving test bound drop_opens_popup
11:38:26     INFO -  355 INFO Entering test bound dont_rollup_oncaretmove
59 failures in the last week:
https://brasstacks.mozilla.com/orangefactor/index.html?display=Bug&bugid=1397728

this is on windows 7 debug (non-e10s- the only configuration we run browser-chrome on non-e10s)

:florian, I see you are the triage owner for Firefox::Search, can you help prioritize this failure and find someone to own fixing this when appropriate?
Flags: needinfo?(florian)
Whiteboard: [stockwell needswork]
Flags: needinfo?(florian) → needinfo?(past)
Mark, would you mind adding this to your queue? I think it's more frequent than any of the other intermittents you are looking at.
Flags: needinfo?(past) → needinfo?(standard8)
Assignee: nobody → standard8
Flags: needinfo?(standard8)
Priority: P5 → P2
Note to self: This could be related to bug 1395871 that landed around the same time as this started, though I can't quite see why...
Here's a try build with a possible fix:

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

If that doesn't work, I think I'll be proposing that we disable this test for Windows on non-e10s.
Florian pointed out that I got the wrong operator in there, so here's a new try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6079669b670002a0e27cd8c6cbe536ff98be4076
It hasn't reproduced on try after about 100 retries. So I think we should probably push this to central and see what happens.

I don't really understand why it needs it in this one specific place in the file, and the rest of the locations are fine, but it does seem to make the test happy...
Comment on attachment 8912137 [details]
Bug 1397728 - Use waitForCondition to avoid intermittent failures in browser_searchbar_openpopup.js.

https://reviewboard.mozilla.org/r/183518/#review188954

I almost r+'ed because this is harmless and this part of the test doesn't matter much, but not understanding is frustrating so I searched a little bit for an explanation and I think I found a plausible one.

The only way that waiting can help here is if we have something other than the popupshowing handler that can remove the attribute. The only other code that does it is the 'input' handler at http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/components/search/content/search.xml#882

So I think the intermittent is due to a race between the "input" handler and the popupshowing handler.

Given that the previous dont_consume_clicks test verifies that we don't have this showonlysettings attribute set, it's definitely something within the drop_opens_popup test that causes it to be set. The only codepath that sets it is the popupshowing handler at http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/components/search/content/search.xml#1112 and this will only happen if the searchbar has this attribute already, and that's done by http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/components/search/content/search.xml#325 This means openSuggestionsPanel is called with aShowOnlySettingsIfEmpty = true. There's only one such call and it's at http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/components/search/content/search.xml#569 This is the "mousedown" handler. So... somehow the test causes a mousedown event in the searchbox before we drop something in it. That seems very unwanted.

The only line I see that is likely to generate events is EventUtils.synthesizeDrop(searchIcon, textbox.inputField, ...
Looking at the implementation at http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/testing/mochitest/tests/SimpleTest/EventUtils.js#2274 it turns out the first parameter ('searchIcon' here) is where we start the drag... and starting a drag likely involes a mousedown event.

So, all this long comment to say that before silencing the error with a waitForCondition, I think we should try replacing the first parameter of EventUtils.synthesizeDrop with an element outside the searchbox.
Attachment #8912137 - Flags: review?(florian)
Comment on attachment 8912137 [details]
Bug 1397728 - Use waitForCondition to avoid intermittent failures in browser_searchbar_openpopup.js.

https://reviewboard.mozilla.org/r/183518/#review188954

The theory seems reasonable, and I did see a mousedown in some of the calls that EventUtils.synthesizeDrop does, so I think it could happen.
Comment on attachment 8912137 [details]
Bug 1397728 - Use waitForCondition to avoid intermittent failures in browser_searchbar_openpopup.js.

https://reviewboard.mozilla.org/r/183518/#review189130

Looks good to me, I hope it indeed fixes the intermittent failure :-).
Attachment #8912137 - Flags: review?(florian) → review+
Try seemed happy, though I didn't do a lot of respins. Let's see ;-)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08f5f3a124d3
Use waitForCondition to avoid intermittent failures in browser_searchbar_openpopup.js. r=florian
https://hg.mozilla.org/mozilla-central/rev/08f5f3a124d3
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
I just checked orangefactor - there's nothing marked after the fixes landed, so I believe the fix was successful :-)
Whiteboard: [stockwell needswork] → [stockwell fixed]
You need to log in before you can comment on or make changes to this bug.