Closed Bug 1039263 Opened 5 years ago Closed 5 years ago

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

Categories

(Hello (Loop) :: Client, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla33

People

(Reporter: NiKo, Unassigned)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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
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.
Attachment #8456875 - Flags: review?(dmose)
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
Attachment #8456875 - Flags: review?(dmose) → review+
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.
Attachment #8456875 - Attachment is obsolete: true
Attachment #8457834 - Flags: review?(standard8)
https://hg.mozilla.org/integration/mozilla-inbound/rev/01f973e7aa61
Assignee: nobody → nperriault
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/01f973e7aa61
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8457834 - Flags: review?(standard8)
Given this was a regression, does this need a manual regression test or is the automation coverage sufficient?
Keywords: regression
Whiteboard: [qa?]
(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).
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/
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: anthony.s.hughes
Whiteboard: [qa?]
You need to log in before you can comment on or make changes to this bug.