1039263 - Loop standalone client can't restart a call (regression)

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Steps to reproduce:

- Standalone webapp opens a loop call link
- Clicks "Start the call"
- Accepts media permissions
- Hang up
- Clicks again on "Start the call"

Results:

The new call won't start.

Expected: 

The new call should start.

Technical details:

The console shows an error which is probably related:

> Error: Invariant Violation: getDOMNode(): A component must be mounted to have a DOM node. localhost:3000/content/shared/libs/react-0.10.0.js:17415

Comment 1

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1039263-Fixed-standalone-Loop-UI-couldn-t-restar.patch

Extracted conversation from IRC:

[14:20:25]  Standard8:	 NiKo`: it'd be interesting to know what regressed it
[14:20:38]  NiKo`:	 Standard8: porting it to react
[14:20:43]  Standard8:	 ah
[14:21:02]  NiKo`:	 Standard8: we were starting subscribing to model event when the component was about to mount
[14:21:14]  Standard8:	 weird though, I thought I tested all that
[14:21:16]  NiKo`:	 but the callback assumed that it was mounted already
[14:21:20]  NiKo`:	 race conditions as usual
[14:21:37]  NiKo`:	 so the fix is to start listening when the component is mounted
[14:21:45]  NiKo`:	 so we're sure to have a DOM
[14:21:54]  NiKo`:	 available for the component
[14:21:56]  Standard8:	 ah ok
[14:23:02]  NiKo`:	 actually we were assuming that componentWillMount was the equivalent of Backbone.View's initialize()
[14:23:08]  NiKo`:	 but it's componentDidMount really
[14:23:42]  NiKo`:	 (actually these are two different beasts, but componentDidMount is the closest to what initialize is)

Also, switched to using Backbone.Events#listenTo and Backbone.Events#stopListening for consistency and ease of unregistration.

Comment 2

[Dan Mosedale (:dmose, :dmosedale)]

Comment on attachment 8456875 [details] [diff] [review]
0001-Bug-1039263-Fixed-standalone-Loop-UI-couldn-t-restar.patch

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

Looks good; thanks for the patch.  r=dmose

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1039263-Fixed-standalone-Loop-UI-couldn-t-restar-rev2.patch

Addressed comments from Mark:

[09:46:06]  Standard8:	 NiKo`: there's an issue
[09:46:27]  Standard8:	 this.TypeError: event is undefined views.js:289
[09:46:32]  Standard8:	 s/this.//
[09:46:54]  Standard8:	 NiKo`: looks like the streamCreated listener

Added missing tests for this to prevent further unnoticed regressions.

Comment 4

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01f973e7aa61

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/01f973e7aa61

Comment 6

[Anthony Hughes (:ashughes) [QA]]

Given this was a regression, does this need a manual regression test or is the automation coverage sufficient?

Comment 7

[Mark Banner (:standard8)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #6)
> Given this was a regression, does this need a manual regression test or is
> the automation coverage sufficient?

This would be covered by a manual smoketest by making a call, or by the functional tests once they are automated (we have some functional tests in the tree now, but we've still got to automate them).

Comment 8

[Anthony Hughes (:ashughes) [QA]]

I confirm that this is working as expected in Firefox 33.0a2 and 34.0a1.

Can someone please review the following smoketest to confirm it's sufficient?
https://moztrap.mozilla.org/manage/case/14532/