Marco Castelluccio [:marco] Reporter
	Description • 11 years ago

See also bug 835111.

	u279076
	Comment 1 • 11 years ago

Thanks Marco, just carrying over the fact that I cannot reproduce any bad behaviour here. Chat windows always open in the currently active window for me. Please provide more context as to what exactly is happening in your case.

	Marco Castelluccio [:marco] Reporter
	Comment 2 • 11 years ago

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #1)
> Thanks Marco, just carrying over the fact that I cannot reproduce any bad
> behaviour here. Chat windows always open in the currently active window for
> me. Please provide more context as to what exactly is happening in your case.

With two windows open, the chat boxes are always opened in the last opened window and not in the currently active window.

I have these three addons installed:
Adblock Plus 2.2.4
DOM Inspector 2.0.14
HTTPS-Everywhere 3.2.1

	u279076
	Comment 3 • 11 years ago

Do these problems reproduce without those add-ons installed?

	Marco Castelluccio [:marco] Reporter
	Comment 4 • 11 years ago

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #3)
> Do these problems reproduce without those add-ons installed?

Yes.
I've tried with a clean profile, but in that case I'm in the bug 872413 situation.

	u279076
	Comment 5 • 11 years ago

Yeah, so I think we need to figure out and resolve bug 872413 before we can move forward with *this* bug. I don't think it's reasonable to try debugging this until that issue is resolved.

	Mark Hammond [:markh] [:mhammond]
	Comment 6 • 11 years ago

I can reproduce this on Linux.  Works fine on Windows (and presumably also on Mac, but I haven't tried that recently).

Note that if the sidebar is open, the chat opens in *all* windows currently - but this is a "feature" of facebook rather than the social api - so to test this specific case, ensure the sidebar is closed as an incoming message arrives.

	u279076
	Comment 7 • 11 years ago

Thanks Mark, can you please clarify your steps to reproduce?

	Mark Hammond [:markh] [:mhammond]
	Comment 8 • 11 years ago

STR:
* Open 2 top-level windows, ensure FB social activated with sidebar closed.
* Bring one of the windows to the foreground.
* Send message from another computer to the FB user logged in to the machine under test.
* Close the message in the window where it appeared.
* Bring the second window to the foreground.
* Send another message.

In one of the 2 cases the window not in the foreground will have got the message.

	u279076
	Comment 9 • 11 years ago

I can't seem to reproduce this with the steps provide, Mark. Everytime the chat appears in my currently active window. Given you can reproduce can you investigate further? Is this a regression we introduced at some point (ie. does it go back to Firefox 17)? or is this an issue with Facebook's implementation?

	Mark Banner (:standard8)
	Comment 10 • 11 years ago

This affects us on Mac with Talkilla as well. My STR are:

1) Open sidebar, open a new window
2) Go back to the first window.
3) Click on a user to open conversation window.

Conversation window appears in second window, not the first one (and there's no indication it has done so).

This appears to be a platform limitation for Mac and Linux, as can be seen in the code:

http://hg.mozilla.org/mozilla-central/annotate/17a47dcef75d/toolkit/components/social/MozSocialAPI.jsm#l281

	Mark Hammond [:markh] [:mhammond]
	Comment 11 • 11 years ago

(In reply to Mark Banner (:standard8) from comment #10)
> This appears to be a platform limitation for Mac and Linux, as can be seen
> in the code:
> 
> http://hg.mozilla.org/mozilla-central/annotate/17a47dcef75d/toolkit/
> components/social/MozSocialAPI.jsm#l281

I was just writing a reply indicating why I thought that code was OK, when I discovered a subtlety I'd missed.  getEnumerator() is documented as iterating "from the oldest window to the youngest" - which I'd always read as "from the least recently used to the most recently used" - but that's not what it does...

It looks like we want an enumerator based on the loop used for getMostRecentWindow, at http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsWindowMediator.cpp#297 - but it doesn't seem to exist and |mTimeStamp| (which would allow us to fix this in js) doesn't seem to be exposed...

	Mark Hammond [:markh] [:mhammond]
	Comment 12 • 11 years ago

hmmm - http://mxr.mozilla.org/mozilla-central/search?string=BROKEN_WM_Z_ORDER implies things might actually be fixed. <... bugzilla searches...>

OK - maybe not fixed, but looks like maybe getting traction - see bug 462222.  I think the simplest is just for us to block on that and drop the BROKEN_WM_Z_ORDER code completely...

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 13 • 11 years ago

I don't see any traction in that bug. It seems to me that we need to fix the social BROKEN_WINDOW_Z_ORDER path to just use the last suitable window in the iterator (similar to what e.g. http://hg.mozilla.org/mozilla-central/annotate/582ffcd0459a/browser/modules/RecentWindow.jsm#l49 does).

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 14 • 11 years ago

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13)
> It seems to me that we need to fix the
> social BROKEN_WINDOW_Z_ORDER path to just use the last suitable window in
> the iterator

Mark pointed out on IRC that it already does this; I misread the code. The bug is just a reflection of the fact that that fallback behavior is not equivalent to the getZOrderDOMWindowEnumerator code, and indeed we would need to remove it (and find an alternative that is equivalent, if it's not getZOrderDOMWindowEnumerator) to fix this.

	Mark Hammond [:markh] [:mhammond]
	Comment 15 • 11 years ago

assigning to myself so I don't forget to test how things work without the work-around.  I'll probably unassign if it doesn't work though :)

	Florian Quèze [:florian] Assignee
	Comment 16 • 11 years ago

I tested this today. On Mac things work well without the workaround.
Unfortunately, on Linux getZOrderDOMWindowEnumerator is still broken (it returned an empty enumerator, which caused social api to fail to open a chat window). However, getMostRecentWindow works on all platforms, so we can use it for the common case, and keep the existing workaround for the unfortunate case of the foreground window being a popup on Linux.

Something similar already exists at http://hg.mozilla.org/mozilla-central/annotate/4887845b1142/browser/components/sessionstore/src/SessionStore.jsm#l3578

	Mark Hammond [:markh] [:mhammond]
	Comment 17 • 11 years ago

Comment on attachment 795522 [details] [diff] [review]
Patch

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

We have an existing test testChatWindowChooser() in browser_social_chatwindow, but obviously it's not very good :)  I suspect the test simply hits the case that works (ie, we create a new window and test that - it should also re-activate the first and check the chats re-appear there.  I suspect that would then cause a failure on Mac *and* Linux.  You patch should then fix Mac and leave Linux broken, which we can handle with a "todo" in the test.

Any chance of looking into the above to ensure we have test coverage for this change?

	Florian Quèze [:florian] Assignee
	Comment 18 • 11 years ago

Added test coverage. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ca572a89b2b9

	Mark Hammond [:markh] [:mhammond]
	Comment 19 • 11 years ago

Comment on attachment 796266 [details] [diff] [review]
Patch v2

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

Awesome, thank you!

	Florian Quèze [:florian] Assignee
	Comment 20 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/28d046b9513e

	Ed Morley [:emorley]
	Comment 21 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/28d046b9513e

	Florian Quèze [:florian] Assignee
	Comment 22 • 11 years ago

Comment on attachment 796266 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Social API chat windows are hardly usable on Mac/Linux if the user has several browser windows, as they can randomly be opened on background windows.
Testing completed (on m-c, etc.): The patch has been on m-c for almost two weeks, and has an automated test.
Risk to taking this patch (and alternatives if risky): Low. If we weren't so late in the cycle, I would request approval-beta too, as it's one of the top issues encountered while dogfooding Talkilla.
String or IDL/UUID changes made by this patch: none.

	Alex Keybl [:akeybl]
	Comment 23 • 11 years ago

Comment on attachment 796266 [details] [diff] [review]
Patch v2

Thanks for the rationale - no issue with this skipping up the trains.

	Florian Quèze [:florian] Assignee
	Comment 24 • 11 years ago

Pushed this to try above mozilla-aurora's current tip to check that the patch passes unit tests there:
https://tbpl.mozilla.org/?tree=Try&rev=aed02a848f40

	Florian Quèze [:florian] Assignee
	Comment 25 • 11 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> Pushed this to try above mozilla-aurora's current tip to check that the
> patch passes unit tests there:
> https://tbpl.mozilla.org/?tree=Try&rev=aed02a848f40

This didn't pass. Turns out http://hg.mozilla.org/mozilla-central/rev/d0064759d817 which landed on m-c before my patch here but after the latest uplift changes the test code a bit.

I'm attaching a fixed patch for aurora, and pushed it to try again: https://tbpl.mozilla.org/?tree=Try&rev=1b687df66ff2

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/d3fd02421f32

	Petruta Horea [:phorea], Desktop QA
	Comment 27 • 11 years ago

I could reproduce the issue on Linux 64-bit with Fx 23.0.1 using STR from Comment 8.
Verified fixed on Fx 25 beta 4 (several scenarios).

	Petruta Horea [:phorea], Desktop QA
	Comment 28 • 11 years ago

Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on latest Aurora 26.0a2 (buildID: 20131002004002)