1106637 - Check that the call node is still present before removing it

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Anshul]

STR

1. Place an outgoing call and accept the call on the other end
2. Place an incoming call
3. Answer the incoming call. The previous call is now held and the incoming (call waiting) call is now connected
4. Merge the calls in a conference.
5. End the conference call.

Observed: After step 5 I see the following exception in the logs
JavascriptException: JavascriptException: TypeError: node.parentNode is null at: app://callscreen.gaiamobile.org/gaia_build_defer_index.js line: 180. The exception is coming from removeCall function in call_screen.js

Comment 1

[Anshul]

This exception causes all our unit test scripts that deal with conference calling to fail.

Comment 2

[bhavana bajaj [:bajaj]]

Let me NI drs from gaia dialer, to get started here.

Comment 3

[Johan Lorenzo [:jlorenzo]]

The issue is not present in today's 2.1:
Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  39
FW-Date         Thu Oct 16 18:19:14 CST 2014
Bootloader      L1TC00011880

Comment 4

[Johan Lorenzo [:jlorenzo]]

Per comment 0.

Comment 5

[Johan Lorenzo [:jlorenzo]]

Triage: Regression preventing unit tests to run.

Comment 6

[Anshul]

Is there any update on this issue?

Comment 7

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Pull request 27411

Hi! After considering other much more complex alternatives consisting on getting the removeCall() function called just once, I opted for the simplest and in my opinion also legit solution based on checking if |node.parentNode| is null before removing the node from it.

The reason to discard these other solutions is that they would force us to keep additional status information about the calls making things error prone without a real need for it. For example, right now and considering the workflow of events being notified (callschanged and statechange) there is no certain way to know if a call participating in a conference has been hang up because the conference was finished or because this precise call was finished, without this additional status information, of course.

Comment 8

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8549551 [details] [review]
Pull request 27411

This LGTM though the reason we need it is pretty clunky; it would be a lot better if we could unify the UI code so we're not trying to remove the same node from two different places. Also please change the patch title before landing to describe what we're doing here such as "before removing a call from the UI verify that the corresponding node is still present", or something along the lines.

Comment 9

[Anshul]

Comment on attachment 8549551 [details] [review]
Pull request 27411

This patch fixes the issue.

Comment 10

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/07062dc2c273b915b70c94cd99677d093038583f

Comment 11

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8549551 [details] [review]
Pull request 27411

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Unknown.
[User impact] if declined: Errors in tests.
[Testing completed]: Gabriele and Germán tested it.
[Risk to taking this patch] (and alternatives if risky): Low.
[String changes made]: None.

Comment 12

[bhavana bajaj [:bajaj]]

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #11)
> Comment on attachment 8549551 [details] [review]
> Pull request 27411
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): Unknown.
> [User impact] if declined: Errors in tests.
> [Testing completed]: Gabriele and Germán tested it.
> [Risk to taking this patch] (and alternatives if risky): Low.
> [String changes made]: None.

gtoro/drs , can we add unit and integration tests around this ?

Comment 13

[Germán Toro del Valle (:gtorodelvalle)]

Redirecting the question to Gabrielle since I am not currently aware of any support to make or to simulate conference calls in integration tests which is the scenario where this patch applies. I mean regarding the UI of conference calls. Thanks!

Comment 14

[Gabriele Svelto [:gsvelto]]

Unfortunately it's impossible right now. When we'll get proper integration with the emulator we might be able to.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/3705a7be3d15cfedabeb2947593ee986cd472101