951188 - [RTSP][V1.3] No error notification when the RTSP link fails to load

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[William Hsu [:whsu]]

* Description:
  This problem happened on v1.3 build.
  Video app keeps loading when the RTSP streaming is offline

* Reproduction steps:
  1. Connect to a hotspot
  2. Launch the following page via browser
     - http://goo.gl/lE2eE3
  2. Tap the "Fake link"

* Expected result:
  Video app pops up a warning message

* Actual result:
  Video app keeps loading

* Reproduction build: V1.3 (Buri)
 - Gaia:     dca0a3dcf062ce3e422a9c56d141c14543c816fb
 - Gecko:    http://hg.mozilla.org/releases/mozilla-aurora/rev/1f7db4cc788e
 - BuildID   20131217004001
 - Version   28.0a2

Comment 1

[William Hsu [:whsu]]

Attachment: WP_20131218_003.mp4

Comment 2

[William Hsu [:whsu]]

Attached the video

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

Strange, we still have onerror callback hooked. In this case, we should receive an error, MediaError.MEDIA_ERR_DECODE or MEDIA_ERR_SRC_NOT_SUPPORTED, through onerror callback. If it is, we can display the error message.

Comment 4

[Hema Koka [:hema]]

NI Sri for confirming blocking decision (per some recent email from Sri/Chris - video streaming is not a blocker for 1.3)

Comment 5

[Sri Kasetti]

Not a blocker for 1.3

Comment 6

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Nominated to 1.3?, as actually RTSP streaming function was landed and accessible in 1.3. We will triage to see if this defect is must-fix or not.

Comment 7

[Hema Koka [:hema]]

wesley, please triage, we are ignoring this one in the media triage (1/15)

Comment 8

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

defer enhancement to 1.4

Comment 10

[Jason Smith [:jsmith]]

I'm reworking the title here to the actual problem, as the dupe & discussion offline indicates that this happens across the board with any type of error when failing to load a RTSP URL in the video app.

Comment 11

[Jason Smith [:jsmith]]

This happens with audio RTSP URL links as well. I feel strongly that this is bad UX - right now, on any failure to load a RTSP URL for any reason, we'll start infinitely spinning forever in the video app without any indication to the user that the RTSP URL failed to load. A user won't have any knowledge of the fact that there is a problem here, so they'll be confused on what's causing the issue - is it a broken RTSP link? Are you offline? What happened?

This violates the principal of gulf of evaluation - http://en.wikipedia.org/wiki/Gulf_of_evaluation.

needinfo on UX to weigh in on the opinion of the severity of this bug

Comment 12

[Stephany Wilkes]

This is, as Jason points out, abysmal UX. During user testing, we see high user frustration when users do not understand *why* something will not work, such as why they've lost network connectivity, cannot send a text message, or cannot view a web page. Loading a video is considered pretty basic functionality, and users also use video a lot to stream and just listen to music without actually watching the video necessarily (Colombia research).

I am concerned about the impact of this on battery life and processing power: does it drain the battery or slow other things down? How easily can the user get out of this state to do something else in the same place?

UX preference would be to block on any of the above, and definitely on all of them.

Comment 14

[Preeti Raghunath(:Preeti)]

We'd like to have an error message that is thrown to indicate to the user that an error has occurred.

Flagging UX to see how best to proceed.

Comment 15

[Axel Hecht [:Pike]]

Flagging this as late-l10n, 'cause it sounds like all this wants is a user message.

Comment 16

[Ethan Tseng [:ethan]]

There are two root causes of this bug:

1. Lack of application timeout for TCP connect 
Our RTSP client uses the general method of non-blocking TCP connect, i.e., calling select() every 1ms to check whether the TCP connection is established or not. However, it doesn't have an application level timeout mechanism to abort connect(). The actual timeout depends on the maximum TCP connect timeout value.
Since our Linux kernel sets /proc/sys/net/ipv4/tcp_syn_retries = 5, the timeout duration is around 180 seconds.

2. Lack of reporting error code from RTSP connection component
Right now the RTSP connection component does not report error code to its protocol listener.

In order to prompt an error message when an RTSP link fails to load, we need to resolve both these two issues.

Comment 17

[Ethan Tseng [:ethan]]

Attachment: bug-951188-wip.patch

What I did in this patch:
1. Set up error code in messages transmitted in RTSPConnectionHandler and ARTSPConnection.
   The purpose is to report an error code to RTSPSource::onDisconnected() in order to distinguish the normal and abnormal disconnected conditions.
2. Add a timeout mechanism for TCP connect. The timeout value is around 10 seconds.

Comment 18

[Ethan Tseng [:ethan]]

Attachment: network_error_message.png

I tested this patch with the following cases:
1. Normal case (playing RTSP audio streaming)
2. Failed to establish RTSP session (host is unreachable or port is not listening)
3. RTSP session established, but failed to load the file (such as typo of the filename)

In case 2, an error message will prompt after 10 seconds.
In case 3, RTSP server will return an error and an error message will prompt quickly.

In either of these two cases, the error message on UI is the same: "A network error caused the video failed to download." The attachment is the captured screen.

Comment 19

[Axel Hecht [:Pike]]

This refers back to the existing error message, nice. Removing late-l10n.

In case you're wondering about the copy in the screenshot, that's bug 892950, fixed on master.

Comment 20

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8367244 [details] [diff] [review]
bug-951188-wip.patch

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

Looks good: r=me. A couple of minor changes requested to add mNumSelectTimeoutRetries = 0 in two places. Please make those changes and retest before landing.

::: netwerk/protocol/rtsp/rtsp/ARTSPConnection.cpp
@@ +386,5 @@
> +            // and reply an error to RTSPConnectionHandler.
> +            mNumSelectTimeoutRetries = 0;
> +            reply->setInt32("result", -ETIMEDOUT);
> +            reply->post();
> +        }

This looks like a decent timeout mechanism to me. select() waits for 1ms each time it is called, and there are 10,000 retries, so about 10s for a connection timeout. And the thread should never block for too long on select, which seems good too.

Please add a log statement in the else branch.

@@ +391,4 @@
>          return;
>      }
>  
>      int err;

Just to be safe, please set mNumSelectTimeoutRetries = 0 after we have made it through the retry mechanism.

Also, I think you should (re-)initialize it before the first call to onCompleteConnection ... maybe in onConnect?

Comment 21

[Ethan Tseng [:ethan]]

Attachment: bug-951188-fix.patch

Fixed the issues mentioned in the review.

Comment 22

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #20)
> Comment on attachment 8367244 [details] [diff] [review]
> Just to be safe, please set mNumSelectTimeoutRetries = 0 after we have made
> it through the retry mechanism.
When onCompleteConnection timed out connect, it posts a reply message back to RTSPConnectionHandler. And I added a mConn->disconnect() call in RTSPConnectionHandler for all the cases except for result = OK. So finally all the error cases will call ARTSPConnection::performDisconnect(). I added mNumSelectTimeoutRetries = 0 in this function.

> Also, I think you should (re-)initialize it before the first call to
> onCompleteConnection ... maybe in onConnect?
Yes! Thanks for your reminder. I added it in onConnect() before the first call to onCompleteConnection.

Comment 23

[jachen]

Clearing UX needs info as it seems like the issue has been resolved with the error messages recommended.

Comment 24

[Ethan Tseng [:ethan]]

Attachment: bug-951188-fix-v2.patch

Added a minor change to improve robustness. In RTSPSource::onDisconnected(), add a check for mLooper and mHandler.

Comment 25

[Ethan Tseng [:ethan]]

The result of try server:
https://tbpl.mozilla.org/?tree=Try&rev=c49474fb9ca8

Comment 26

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #18)
> I tested this patch with the following cases:
> 1. Normal case (playing RTSP audio streaming)
> 2. Failed to establish RTSP session (host is unreachable or port is not
> listening)
> 3. RTSP session established, but failed to load the file (such as typo of
> the filename)
> In case 2, an error message will prompt after 10 seconds.
> In case 3, RTSP server will return an error and an error message will prompt
> quickly.

Add one more case:
Case 4: Turn off Wifi while RTSP streaming is being played.
The network error message will prompt immediately.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11fe654f16d8

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11fe654f16d8

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b39537be627

Comment 30

[sarsenyev]

Verified fixed on the latest master build
The error message notifies user about the broken link

Device: Buri 1.4 MOZ
BuildID: 20140219040204
Gaia: ac06cfbd2baf6494ffbb668cc599e3892cd5e17b
Gecko: bf0e76f2a7d4
Version: 30.0a1
Firmware Version: v1.2-device.cfg

Comment 31

[William Hsu [:whsu]]

Thanks for the help.
Good Job! Video app pops up a warning message

* Build Information:
 - Gaia      a43904d9646109b48836d62f7aa51e499fbf4b2e
 - Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/32fd5d798477
 - BuildID   20140219164003
 - Version   28.0

* Test Result:
 - Cannot reproduce

Also, attaching the screenshot (2014-02-20-21-13-38.png). FYI.

Comment 32

[William Hsu [:whsu]]

Attachment: 2014-02-20-21-13-38.png

Comment 33

[u279076]

Jason, does this need any testing on Desktop?

Comment 34

[Jason Smith [:jsmith]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #33)
> Jason, does this need any testing on Desktop?

Nope - this only applies to FxOS.

Comment 35

[u279076]

(In reply to Jason Smith [:jsmith] from comment #34)
> Nope - this only applies to FxOS.

Thanks Jason. I'm marking this [qa-] for Desktop Firefox since it's already been marked verified for FxOS.