Closed Bug 1090633 Opened 10 years ago Closed 9 years ago

Intermittent browser_chatwindow.js | first window got new chat - 0 == 1 - JS frame :: browser_chatwindow.js :: testChatWindowChooser :: line 116 | first window got the chat

Categories

(Firefox Graveyard :: SocialAPI, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(firefox35 fixed, firefox36 fixed, firefox37 fixed, firefox38 fixed, firefox38.0.5 fixed, firefox39 fixed, firefox40 fixed, firefox-esr31 unaffected, firefox-esr38 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- fixed

People

(Reporter: KWierso, Assigned: mixedpuppy)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 1 obsolete file)

More likely a social api based issue.
Component: Client → SocialAPI
Product: Loop → Firefox
QA Contact: anthony.s.hughes
running a try with a possible fix here:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=149d52397a09

At least on my linux vm, I was able to 100% reproduce this intermittent, and the patch resolved it.
Attached patch test fix (obsolete) — — Splinter Review
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=149d52397a09

Looks like it's ok with this patch.
Assignee: nobody → mixedpuppy
Attachment #8534492 - Flags: review?(mhammond)
Comment on attachment 8534492 [details] [diff] [review]
test fix

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

::: browser/base/content/test/chat/head.js
@@ +64,5 @@
>    waitForFocus(deferred.resolve);
>    return deferred.promise;
>  }
>  
> +function waitForCondition(condition, errorMsg) {

TBH I'd prefer is this was in the test itself so there's less chance of it being used as the oversized hammer that it is, with a comment in the test saying we'd prefer to avoid using it using mutation observers or something.

But yeah, good enough to fix this orange!
Attachment #8534492 - Flags: review?(mhammond) → review+
Attached patch test fix — — Splinter Review
moved wait code from head to test file, carry forward r+

https://hg.mozilla.org/integration/fx-team/rev/9612347414cb
Attachment #8534492 - Attachment is obsolete: true
Attachment #8534639 - Flags: review+
Comment on attachment 8534639 [details] [diff] [review]
test fix

Approval Request Comment
[Feature/regressing bug #]: test failure
[User impact if declined]: none
[Describe test coverage new/current, TBPL]: 
[Risks and why]: none
[String/UUID change made/needed]: none

this fix should uplift to aurora to fix the intermittent there as well.
Attachment #8534639 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/9612347414cb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Attachment #8534639 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/15384e0cd4d4

FYI, test-only changes can land without approval. But it doesn't hurt to ask anyway :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
See Also: → 1135545
Not sure who's working on SocialAPI these days, but this is happening with too great of a frequency to be ignored. Per the Test Disabling Policy [1], I intend to disable this test in a couple days if someone doesn't pick it up.

[1] https://wiki.mozilla.org/Sheriffing/Test_Disabling_Policy
Flags: needinfo?(mixedpuppy)
FTR, this is easy to repro for me locally - run the test with --run-until-failure and as soon as the test starts, switch focus to some other app. The test then immediately fails.

I think there's a problem whereby a window isn't made the "most recent" window after focus when Fx itself doesn't have focus. In the cases when this fails, we are in a state where |Services.wm.getMostRecentWindow("navigator:browser") != window| even though |Services.focus.activeWindow == window|.  Doing a waitForCondition for window to be the most-recent is better, but fails within a minute or so rather than immediately.

IOW, we are managing to get into a state where |Services.focus.activeWindow != Services.wm.getMostRecentWindow("navigator:browser")|, which sounds like a bug in the window manager.  We could probably "fix" this by having Chat.jsm consider the activeWindow before the most recent, but I think open that window manager bug first and see what we learn.
This is a test-only change.

As we chatted about, this patch changes browser_chatwindow.js to just call findChromeWindowForChats rather than trying to ensure the chat actually opens in a window - this avoids focus changing while the test is running from breaking things.

A try with that change then showed some orange in browser_focus.js, and I fixed that by not assuming the window still has focus.

The browser_focus orange was only seen on Linux on earlier try runs. A linux-only try is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f5781869086, which has many retriggers and is green.
Flags: needinfo?(mixedpuppy)
Attachment #8599717 - Flags: review?(mixedpuppy)
Attachment #8599717 - Flags: review?(mixedpuppy) → review+
https://hg.mozilla.org/mozilla-central/rev/a375bdb7fb5d
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 37 → Firefox 40
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.