Closed Bug 1113574 Opened 9 years ago Closed 9 years ago

UiTour: Hello FTE still thinks conversation view was open even after it is closed

Categories

(Firefox :: Tours, defect)

37 Branch
defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 37
Iteration:
37.3 - 12 Jan
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: agibson, Assigned: MattN)

References

Details

Attachments

(3 files, 2 obsolete files)

This bug is not reproducible every time, but I have hit it during testing enough times to figure out how to trigger it.

Note: I am testing with the custom build linked here: https://bugzilla.mozilla.org/show_bug.cgi?id=1080953#c31

STR:

1.) Open the Hello FTE which is currently on demo2 [1]
2.) When the hello panel is opened, click "Start a conversation"
3.) When the conversation view opens, quickly click the "copy" button, and then close the conversation view (as quickly as you can) before the room has properly connected.
4.) Close the Hello FTE tab
5.) Paste the copied URL into another browser and click "Join the conversation" to start the video conversation.

Expected results

The Hello icon should turn blue and wait for the user to click the icon before re-opening the Hello FTE tab

Actual results

The FTE tab reopens automatically with the &incomingConversation=open param, resulting in the page displaying the incorrect state (see attachment).

The page can also end up getting a 'Loop:IncomingConversation' event with `data.conversationOpen: true` at an incorrect time, if you follow the above steps but leave the FTE tab open.

I think it might be some sort of race condition between a room opening and the conversation view being closed. I've triggered it accidentally a few times but can reproduce it quite frequently if I try the above steps :/

[1] https://www-demo2.allizom.org/en-US/firefox/37.0a1/hello/start/?utm_source=firefox-browser&utm_medium=firefox-browser&utm_campaign=settings-menu
Just for clarity, the URL I am setting for loop.gettingStarted.url is:

https://www-demo2.allizom.org/%LOCALE%/firefox/%VERSION%/hello/start/
I'll try to reproduce this.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Hardware: x86 → All
Alex, can you double-check you can repro this with today's Nightly?
Attached image loop-six-tabs-wrong-state.png (obsolete) —
I just tried in today's Nightly and managed to hit the bug after several attempts. What's even weirder it opened 6 tabs all at once on the final try (see attachment) :(
As requested by Matt, I managed to get a quick screen-cast of the bug:

https://drive.google.com/a/mozilla.com/file/d/0B5Zlfi6zV6UraW1lMmZvaDZtQWc/view?usp=sharing

Log with loop debugging enabled is here: http://pastebin.mozilla.org/8077005

As noted in our IRC discussion, when the bug occurs the hello icons stays blue even when the room is closed.

Sometimes I have to try fast to hit this bug, but I've also run into it several times by accident without really trying purposefully.
I think I understand what is happening but I'm having a hard time reproducing. I have ideas for workarounds though.
Attached file MozReview Request: bz://1113574/MattN (obsolete) —
Attachment #8539636 - Flags: review?(mixedpuppy)
Attachment #8539636 - Flags: review?(dmose)
/r/1613 - Bug 1113574 - Check that the room <chatbox> is still open before resuming the tour from a join notification. r=dmose,mixedpuppy

Pull down this commit:

hg pull review -r 989df0f6a79f82c5527b4c6e1aec998fa8ef5456
The room participant list only gets updated after getting a push notification and this can lead to the Rooms store still showing the user in their room even after they have left. To avoid this, we check that the <chatbox> for the room is actually still present.

We want this to land ASAP so I'm putting this up for review without tests and they can be reviewed separately.
Attachment #8539636 - Flags: review?(standard8)
/r/1613 - Bug 1113574 - Check that the room <chatbox> is still open before resuming the tour from a join notification. r=dmose,mixedpuppy

Pull down this commit:

hg pull review -r 989df0f6a79f82c5527b4c6e1aec998fa8ef5456
Attachment #8539381 - Attachment is obsolete: true
Alex, here is a try build with this patch and the one for bug 1112525. Can you let me know if it fixes both bugs?

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@noorenberghe.ca-d5a9a5225b8a/
Flags: needinfo?(agibson)
Many thanks for looking into this Matt,

This build seems to fix Bug 1112525 (I've left a comment over there to confirm).

I've tested this bug and this is what I found:

When the Hello icon remains blue after closing a room, the tour did not go into the wrong state when the conversation connects. The page correctly prompted me that a conversation was waiting, I clicked the icon and the FTE tab opened in the right state (hooray!). When I click to connect the conversation, the rooms window opens but says "Something went wrong" (see attached). I'm guessing this is a loop issue more than a UITour thing? Seems to happen consistently when this edge-case occurs.

One change in behavior for the tour that I do see here which could be an issue, is I no longer get a 'Loop:IncomingConversation' event with `data.conversationOpen: true` once the user clicks the conversation in the rooms list on the final step (when the door-hanger says "you have a conversation waiting"). So with this build I don't get the ending "success" message state. I double-checked and I do get the event in the current Nightly.

Hope that makes sense
Flags: needinfo?(agibson)
(In reply to Alex Gibson [:agibson] from comment #13)
> When the Hello icon remains blue after closing a room, the tour did not go
> into the wrong state when the conversation connects. The page correctly
> prompted me that a conversation was waiting, I clicked the icon and the FTE
> tab opened in the right state (hooray!). When I click to connect the
> conversation, the rooms window opens but says "Something went wrong" (see
> attached). I'm guessing this is a loop issue more than a UITour thing? Seems
> to happen consistently when this edge-case occurs.

Yeah, we should get a Loop bug on file for this. After some minutes I believe it will timeout and the button will go grey again.

> One change in behavior for the tour that I do see here which could be an
> issue, is I no longer get a 'Loop:IncomingConversation' event with
> `data.conversationOpen: true` once the user clicks the conversation in the
> rooms list on the final step (when the door-hanger says "you have a
> conversation waiting"). So with this build I don't get the ending "success"
> message state. I double-checked and I do get the event in the current
> Nightly.

Now that we discussed on IRC, I understand what you're saying. This is how I intended for it to work from the beginning. Either you get a new tab with the query parameter or you get one 'Loop:IncomingConversation' event on an existing tab. You should use the Loop:ChatWindowOpened notification to know to go to the next step after the room list.
Attachment #8539636 - Flags: review?(dmose) → review+
https://reviewboard.mozilla.org/r/1611/#review1055

Looks good.  r=dmose once the the testing issue is fixed.

::: browser/components/loop/MozLoopService.jsm
(Diff revision 1)
> +      log.debug(chatbox.src, chatbox);

Maybe ditch the log.debug?

::: browser/components/loop/MozLoopService.jsm
(Diff revision 1)
> -    // If expiresTime is not in the future and the user hasn't
> +    if (!chatboxesForRoom.length) {

Please file a bug on fixing the rooms data caching architecture so that this can't happen.  (Maybe just by making the rooms architecture use the gDirty bit, if not actually separating out the caching concern from the other room store concerns).

::: browser/modules/Chat.jsm
(Diff revision 1)
> +        [yield c for (c of chatbar.children)];

If there's not test coverage that would catch a change in chatbot.children the implementation detail, can you either add that or at least an XXX comment in the implemention so that the next editor of that code knows to tweak here as well.

::: browser/modules/Chat.jsm
(Diff revision 1)
> +   */

Please fix the test problem that showed up in the try build.  Current theories include:

* odd interaction between JS getter and the generator being returned
* because we're calling close() inside the iterator, this means that the thing being iterated over is no longer stable, which might cause surprising behavior.

::: browser/components/loop/MozLoopService.jsm
(Diff revision 1)
> +  getChatWindowID: function(conversationWindowData) {

Nice cleanup working hoisting these things into their own functions.
https://reviewboard.mozilla.org/r/1611/#review1059

> Please fix the test problem that showed up in the try build.  Current theories include:
> 
> * odd interaction between JS getter and the generator being returned
> * because we're calling close() inside the iterator, this means that the thing being iterated over is no longer stable, which might cause surprising behavior.

This ended up being the latter since .children is a live NodeList. The old code dealt with this by creating an array from the NodeList but I got rid of the intermediate object thinking it wasn't necessary. I've now added a comment about why we're making the array.
Attachment #8539636 - Flags: review?(standard8)
Attachment #8539636 - Flags: review?(mhammond)
Attachment #8539636 - Flags: review+
/r/1613 - Bug 1113574 - Check that the room <chatbox> is still open before resuming the tour from a join notification. r=dmose,mixedpuppy

Pull down this commit:

hg pull review -r a17dddabb3c4d27b50a0344f33342d6adb9f7082
See Also: → 1114846
Attachment #8539636 - Flags: review?(mhammond) → review+
Iteration: --- → 37.3
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
Attachment #8539636 - Flags: review?(mixedpuppy)
https://hg.mozilla.org/mozilla-central/rev/df07507d7e78
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Comment on attachment 8539636 [details]
MozReview Request: bz://1113574/MattN

Approval Request Comment
[Feature/regressing bug #]: Hello FTE tour
[User impact if declined]: The tour may get into the wrong state if the user closes a Room and someone joins before the Room info gets updated from the server.
[Describe test coverage new/current, TBPL]: QE will verify the tour before release
[Risks and why]: Low risk mostly adding a check that the room is actually still open instead of just trusting the data.
[String/UUID change made/needed]: None
Attachment #8539636 - Flags: approval-mozilla-beta?
Attachment #8539636 - Flags: approval-mozilla-aurora?
Attachment #8539636 - Flags: approval-mozilla-beta?
Attachment #8539636 - Flags: approval-mozilla-beta+
Attachment #8539636 - Flags: approval-mozilla-aurora?
Attachment #8539636 - Flags: approval-mozilla-aurora+
Verified on today's [1/2] Nightly 37, was unable to repro the issue after numerous attempts using the steps above.
Attachment #8539636 - Attachment is obsolete: true
Attachment #8618939 - Flags: review+
You need to log in before you can comment on or make changes to this bug.