1045062 - [RTSP] Replace CHECK assertions by NS_ASSERTION or graceful assertions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[Ethan Tseng [:ethan]]

In our RTSP client codes (netwerk/protocol/rtsp/rtsp) ported from Android's stagefright library, there are many CHECK assertions, which are defined in ADebug.h.
http://androidxref.com/4.4.4_r1/xref/frameworks/av/include/media/stagefright/foundation/ADebug.h#32

Usually, the failure of an assertion indicates a fatal error and would stop program execution.
However, many assertions under netwerk/protocol/rtsp/rtsp are used to check packet formats. Crashing the whole system seems to be overreacting for such assertion failures.

This bug is aimed to replace such assertions by NS_ASSERTION or other graceful error handles.

Comment 1

[Ethan Tseng [:ethan]]

We had encountered several crash bugs caused by such assertions.
For example:
- Bug 1009497 - [RTSP][V2.0] Crash happened while device plays MP3 stream over RTSP
- Bug 1038037 - [dolphin][flame] b2g crash when open some streaming audio from browser

Comment 2

[Ethan Tseng [:ethan]]

Refer to the suggestion from Steve Workman.
https://bugzilla.mozilla.org/show_bug.cgi?id=1038037#c25

Comment 3

[Ethan Tseng [:ethan]]

According to AOSP, these CHECK assertions are NOT stripped from release builds.
However Android phones wouldn't crash on these assertions.

Definition of CHECK:
http://androidxref.com/4.4.4_r1/xref/frameworks/av/include/media/stagefright/foundation/ADebug.h#32
Definition of LOG_ALWAYS_FATAL_IF:
http://androidxref.com/4.4.4_r1/xref/system/core/include/log/log.h#374

Comment 4

[Jonathan Hao (inactive) [:jhao]]

(In reply to Ethan Tseng [:ethan] from comment #1)
> We had encountered several crash bugs caused by such assertions.
> For example:
> - Bug 1009497 - [RTSP][V2.0] Crash happened while device plays MP3 stream
> over RTSP
> - Bug 1038037 - [dolphin][flame] b2g crash when open some streaming audio
> from browser

I will first use the solutions in the two bugs above to fix similar assertions, i.e. CHECK contating GetAttribute() or getFormat().

Comment 5

[Ethan Tseng [:ethan]]

More reference bugs.
These bugs were also fixed by altering CHECK() assertions to error handles.
* Bug 1059144 - System crash when RTSP client connects to a non-RTSP-server port
* Bug 1049241 - Cannot play some RTSP link due to error in MP4A-LATM assembler
  * Two duplicate bugs:
  * Bug 1055949 - System crash when playing MP4A-LATM audio type over RTSP
  * Bug 1054230 - [RTSP] Potential crash happens if decode error when reading meta data
* Bug 1038037 - B2G crash when opening malformed MP4A-LATM audio streaming over RTSP

Comment 6

[Jonathan Hao (inactive) [:jhao]]

Attachment: bug-1045062-wip.patch

Eight CHECK() down, 298 more to go.

Ethan, could you take a look when you have time to see if I am fixing it in the right way?

Comment 7

[Ethan Tseng [:ethan]]

(In reply to Jonathan Hao [:jhao] from comment #6)
> Created attachment 8493641 [details] [diff] [review]
> bug-1045062-wip.patch 
> Eight CHECK() down, 298 more to go. 
> Ethan, could you take a look when you have time to see if I am fixing it in
> the right way?

Hi Jonathan,
After a quick glance at your patch, yes, I think you are on the right track! :)

Comment 8

[Jonathan Hao (inactive) [:jhao]]

Thanks, Ethan :)

BTW, do you think TRESPASS() should be replaced too? Here is the definition of TRESPASS():
http://androidxref.com/4.4_r1/xref/frameworks/av/include/media/stagefright/foundation/ADebug.h#78
It's almost the same as CHECK(false).

Comment 9

[Ethan Tseng [:ethan]]

(In reply to Jonathan Hao [:jhao] from comment #8)
> BTW, do you think TRESPASS() should be replaced too? Here is the definition
> of TRESPASS():
> It's almost the same as CHECK(false).
Yes, I agree with you.
Although I didn't encounter a crash caused by TRESPASS() under my belt, replacing them by error handling
would be a good idea to reduce the odds of intimidating crash.

Let's keep in mind the guidelines for using assertions:
* Use error-handling code for conditions you expect to occur;
  use assertions for conditions that should never occur.
* Error handling typically checks for bad input data; assertions check for bugs in the code.

Following the guidelines, you can keep TRESPASS()/assertions in the default case of switch-case statements, such as in RTSPConnectionHandler.h and RTSPSource.cpp.
As for TRESPASS() calls used for format checks, replace them by error handling.

Comment 10

[Jonathan Hao (inactive) [:jhao]]

Attachment: bug-1045062-wip.patch

I have replace most format assertions with error-handling, except for:
1. Parameter format check in several constructors, e.g. AAMRAssembler, AMPEG4AudioAssembler, AMPEG4ElementaryAssembler, ... I am not sure how to handle error if an object cannot be successfully constructed.
2. Message format check in OnMessageReceived(), because the message should be sent by the same module, so wrong message formats should be bugs.

Ethan, do you think it's ready for review? Or is there anything else that needs to be done? Thanks a lot.

Comment 11

[Ethan Tseng [:ethan]]

(In reply to Jonathan Hao [:jhao] from comment #10)
> I have replace most format assertions with error-handling, except for:
> 1. Parameter format check in several constructors, e.g. AAMRAssembler,
> AMPEG4AudioAssembler, AMPEG4ElementaryAssembler, ... I am not sure how to
> handle error if an object cannot be successfully constructed.
I can think of two solutions:
1. Use a factory method, such as CreateAssemblerByType(), to do error handling.
2. Add an Init() method to these sub-classes of ARTPAssembler.
   Move those checks from constructors to Init() method.
We could file another bug to deal with this.

> 2. Message format check in OnMessageReceived(), because the message should
> be sent by the same module, so wrong message formats should be bugs.
Yes.

> Ethan, do you think it's ready for review? Or is there anything else that
> needs to be done? Thanks a lot.
Yeah! Most of format check assertions are replaced in your patch.
Let's get it to review.

Comment 12

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8496701 [details] [diff] [review]
bug-1045062-wip.patch

Hi Steve, could you review this patch? Many thanks.

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8496701 [details] [diff] [review]
bug-1045062-wip.patch

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

Thanks for the patch Jonathan. I've given some comments on the core files in this patch, but I think it's better if Ethan does this review first.

I think the patch should be split up as well. There are a lot of changes here (which is a lot of work - thank you!), and I'd want to make sure that the correct error handling is used. I think you'll understand in my comments.

In any case, I think Ethan knows RTSP better than I do and will know how each error should be handled. So, I'm going to delegate the review to him. I'll take a quick look once he thinks it's ok.

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +511,5 @@
>                          ssize_t i = response->mHeaders.indexOfKey("location");
> +                        if (i < 0) {
> +                            LOGE("No location header in response");
> +                            break;
> +                        }

What happens after onMessageReceived returns here? If the 302 has no location header, should we teardown the session? If this happens already, please add a brief comment in the LOGE to say that it's going to happen. I want to make sure that we're not going to ignore the incomplete response and then just wait indefinitely.

@@ +644,5 @@
>                          ssize_t i = response->mHeaders.indexOfKey("session");
> +                        if (i < 0) {
> +                            LOGE("No session header in response");
> +                            break;
> +                        }

Same comment as mission "location" in 302.

@@ +688,5 @@
>                          i = response->mHeaders.indexOfKey("transport");
> +                        if (i < 0) {
> +                            LOGE("No transport header in response");
> +                            break;
> +                        }

Same as "session".

@@ +1162,5 @@
>  
>                          ssize_t i = response->mHeaders.indexOfKey("rtp-info");
> +                        if (i < 0) {
> +                            LOGE("No RTP info in response");
> +                            (new AMessage(kWhatAbort, id()))->post();

"No RTP info in response; aborting"

@@ +1193,5 @@
>                  sp<ABuffer> buffer = static_cast<ABuffer *>(obj.get());
>  
>                  int32_t index;
> +                if (!buffer->meta()->findInt32("index", &index)) {
> +                    break;

Why no LOGW or LOGE here?

@@ +1332,5 @@
>              LOGV("streamInfo[%d] = %s", n, (*it).c_str());
>  
> +            if (!GetAttribute((*it).c_str(), "url", &val)) {
> +                LOGE("No url attribute");
> +                return;

continue? Try to parse the next track? Or should we clear out the track info that was set in this loop?

Should the whole session be torn down?

::: netwerk/protocol/rtsp/rtsp/RTSPTransmitter.h
@@ +270,4 @@
>          ssize_t i = response->mHeaders.indexOfKey("www-authenticate");
> +        if (i < 0) {
> +            return false;
> +        }

Looks like it would be good to have some LOGW statements in this function. It would be useful to know why the authentication failed.

@@ +468,4 @@
>                       << result << " (" << strerror(-result) << ")";
>  
>                  sp<RefBase> obj;
>                  CHECK(msg->findObject("response", &obj));

Maybe you can remove this CHECK.

@@ +472,5 @@
>                  sp<ARTSPResponse> response;
>  
>                  if (result == OK) {
>                      response = static_cast<ARTSPResponse *>(obj.get());
> +                    if (!response.get()) {

I think you can check for obj.get() != where CHECK(msg->findObject ...) used to be, right?
LOGE please.

@@ +485,5 @@
>                  }
>  
>                  ssize_t i = response->mHeaders.indexOfKey("session");
> +                if (i < 0) {
> +                    (new AMessage(kWhatQuit, id()))->post();

I think we should have LOGE statements here, especially if we're going to post a quit message.

Comment 14

[Ethan Tseng [:ethan]]

Comment on attachment 8496701 [details] [diff] [review]
bug-1045062-wip.patch

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

Some existing code is not consistent in log severity.
I suggest to establish some guidelines to choose the severity of logs.
  * Use LOGE() if we cannot play the whole stream, such as failed to connect server, 
    failed to create object, unsupported media format, ..., etc.
  * Use LOGW() if the error might indicate a significant problem.
    For example, frequent malformed packets could result in poor playback quality.
  * Use LOGV() if the error doesn't have significant impact.
    An example is dropping stale packet.
  * Use LOGI() only for printing debugging information.

Meanwhile, replace CHECK() by MOZ_ASSERT() if it is an actual assertion.

::: netwerk/protocol/rtsp/rtsp/AAMRAssembler.cpp
@@ +146,5 @@
> +    if (payloadHeader & 0x0f != 0u) {
> +        queue->erase(queue->begin());
> +        ++mNextExpectedSeqNo;
> +
> +        LOGV("Wrong payload header");

Use LOGW() for malformed packet.

@@ +193,5 @@
> +    if (!CopyTimes(accessUnit, buffer)) {
> +        queue->erase(queue->begin());
> +        ++mNextExpectedSeqNo;
> +
> +        LOGV("CopyTimes() fail");

Remove this log. Centralize the log in CopyTimes().

::: netwerk/protocol/rtsp/rtsp/AAVCAssembler.cpp
@@ +122,5 @@
>      hexdump(buffer->data(), buffer->size());
>  #endif
>  
>      uint32_t rtpTime;
> +    if (!buffer->meta()->findInt32("rtp-time", (int32_t *)&rtpTime)) {

LOGW("Cannot find rtp-time");

@@ +159,5 @@
>          sp<ABuffer> unit = new ABuffer(nalSize);
>          memcpy(unit->data(), &data[2], nalSize);
>  
> +        if (!CopyTimes(unit, buffer)) {
> +            LOGV("CopyTimes() failed");

Remove this log.

@@ +166,3 @@
>  
> +        if (!addSingleNALUnit(unit)) {
> +            LOGV("addSingleNALUnit() failed");

Use LOGW().

@@ +186,5 @@
> +        LOGV("Queue is empty");
> +
> +        ++mNextExpectedSeqNo;
> +        return MALFORMED_PACKET;
> +    }

We should keep this assertion because the purpose of design guarantees the queue should not be empty here (called from ARTPAssembler::onPacketReceived()).
MOZ_ASSERT(!queue->empty());

@@ +203,4 @@
>      unsigned indicator = data[0];
>  
> +    if ((indicator & 0x1f) != 28) {
> +        LOGV("Indicator is wrong");

Use LOGW().

@@ +340,5 @@
>  
> +bool AAVCAssembler::submitAccessUnit() {
> +    if (mNALUnits.empty()) {
> +        return false;
> +    }

MOZ_ASSERT(!mNALUnits.empty());

::: netwerk/protocol/rtsp/rtsp/AH263Assembler.cpp
@@ +83,5 @@
>          return WRONG_SEQUENCE_NUMBER;
>      }
>  
>      uint32_t rtpTime;
> +    if (!buffer->meta()->findInt32("rtp-time", (int32_t *)&rtpTime)) {

LOGW("Cannot find rtp-time. Malformed packet.");

@@ +186,5 @@
>  
> +bool AH263Assembler::submitAccessUnit() {
> +    if (mPackets.empty()) {
> +        return false;
> +    }

The same reason as above. Replace CHECK() by MOZ_ASSERT().

::: netwerk/protocol/rtsp/rtsp/AMPEG4AudioAssembler.cpp
@@ +301,3 @@
>  
>      unsigned numLayer = bits->getBits(3);
> +    if (numLayer == 0u) {

Should be:
if (numLayer != 0u)

@@ +456,5 @@
> +                return NULL;
> +            }
> +            if (offset + (mOtherDataLenBits / 8) > buffer->size()) {
> +                return NULL;
> +            }

Don't return NULL in this function. Set mAccessUnitDamaged = true instead.
if (mOtherDataLenBits % 8 != 0) {
    mAccessUnitDamaged = true;
    return out;
}

@@ +550,5 @@
>          return WRONG_SEQUENCE_NUMBER;
>      }
>  
>      uint32_t rtpTime;
> +    if (!buffer->meta()->findInt32("rtp-time", (int32_t *)&rtpTime)) {

LOGW("Cannot find rtp-time. Malformed packet.");

@@ +557,3 @@
>  
>      if (mPackets.size() > 0 && rtpTime != mAccessUnitRTPTime) {
> +        if(!submitAccessUnit()) {

LOGW("Cannot find rtp-time. Malformed packet.");

@@ +570,5 @@
>      return OK;
>  }
>  
> +bool AMPEG4AudioAssembler::submitAccessUnit() {
> +    if (mPackets.empty()) {

Use MOZ_ASSERT().

::: netwerk/protocol/rtsp/rtsp/AMPEG4ElementaryAssembler.cpp
@@ +216,5 @@
>      }
>  
>      uint32_t rtpTime;
> +    if (!buffer->meta()->findInt32("rtp-time", (int32_t *)&rtpTime)) {
> +

The block body is missing.

@@ +222,3 @@
>  
>      if (mPackets.size() > 0 && rtpTime != mAccessUnitRTPTime) {
> +        if(!submitAccessUnit()) {

LOGW().

@@ +239,5 @@
>          mPackets.push_back(buffer);
>      } else {
>          // hexdump(buffer->data(), buffer->size());
>  
> +        if (buffer->size() < 2u) {

LOGW("Payload format error. Malformed packet.");

@@ +246,3 @@
>          unsigned AU_headers_length = U16_AT(buffer->data());  // in bits
>  
> +        if (buffer->size() < 2 + (AU_headers_length + 7) / 8) {

LOGW("Payload format error. Malformed packet.");

@@ +337,5 @@
>               it != headers.end(); ++it) {
>              mPreviousAUCount++;
>              const AUHeader &header = *it;
>              const AUHeader &first = *headers.begin();
> +            if (offset + header.mSize > buffer->size()) {

LOGW("Payload format error. Malformed packet.");

@@ +354,5 @@
>  
>              mPackets.push_back(accessUnit);
>          }
>  
> +        if (offset != buffer->size()) {

LOGW("Payload format error. Malformed packet.");

@@ +368,5 @@
>  
> +bool AMPEG4ElementaryAssembler::submitAccessUnit() {
> +    if (mPackets.empty()) {
> +        return false;
> +    }

User MOZ_ASSERT().

::: netwerk/protocol/rtsp/rtsp/APacketSource.cpp
@@ +113,5 @@
>          return NULL;
>      }
>  
>      sp<ABuffer> profileLevelID = decodeHex(val);
> +    if (!profileLevelID.get() || profileLevelID->size() != 3u) {

LOGW("Format error in profile-level-id");

@@ +285,5 @@
>      } else {
>          objectType = 0x40;  // Audio ISO/IEC 14496-3
>      }
>  
> +    if (!GetAttribute(params, "config", &val)) {

LOGW("Cannot find attribute config");

@@ +386,5 @@
>      *width = 0;
>      *height = 0;
>  
>      AString val;
> +    if (!GetAttribute(params, "config", &val)) {

LOGW("Cannot find attribute config");

::: netwerk/protocol/rtsp/rtsp/ARTPAssembler.cpp
@@ +68,2 @@
>      uint32_t rtpTime;
> +    if (!from->meta()->findInt32("rtp-time", (int32_t *)&rtpTime)) {

Add a log here and remove repeated logs in other files.
LOGW("CopyTimes: Cannot find rtp-time");

::: netwerk/protocol/rtsp/rtsp/ARTPSession.cpp
@@ +159,4 @@
>              }
>  
>              sp<RefBase> obj;
>              CHECK(msg->findObject("access-unit", &obj));

Replace this CHECK() too.

@@ +164,5 @@
>              sp<ABuffer> accessUnit = static_cast<ABuffer *>(obj.get());
>  
>              uint64_t ntpTime;
> +            if (!accessUnit->meta()->findInt64(
> +                        "ntp-time", (int64_t *)&ntpTime)) {

LOGW("Cannot find ntp-time");

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +511,5 @@
>                          ssize_t i = response->mHeaders.indexOfKey("location");
> +                        if (i < 0) {
> +                            LOGE("No location header in response");
> +                            break;
> +                        }

We should set result as some kind of error code here.
The end of this case would disconnect the session if the result is not OK.

@@ +644,5 @@
>                          ssize_t i = response->mHeaders.indexOfKey("session");
> +                        if (i < 0) {
> +                            LOGE("No session header in response");
> +                            break;
> +                        }

Same as "location".

@@ +688,5 @@
>                          i = response->mHeaders.indexOfKey("transport");
> +                        if (i < 0) {
> +                            LOGE("No transport header in response");
> +                            break;
> +                        }

Same as "location".

@@ +1194,5 @@
>  
>                  int32_t index;
> +                if (!buffer->meta()->findInt32("index", &index)) {
> +                    break;
> +                }

LOGW("Cannot find index");

@@ +1333,5 @@
>  
> +            if (!GetAttribute((*it).c_str(), "url", &val)) {
> +                LOGE("No url attribute");
> +                return;
> +            }

I am not sure "url" is a mandatory attribute or not.
Look up the RFC.

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +598,5 @@
>            meta->SetWidth(int32Value);
>  
> +          if (!format->findInt32(kKeyHeight, &int32Value)) {
> +            return;
> +          }

I suggest to keep all these CHECK() in OnConnected() as assertions.
Any failure of them indicates an internal logic error.
Use MOZ_ASSERT().

@@ +698,5 @@
>      meta = new mozilla::net::RtspMetaData();
>  
> +    if (!accessUnit.get() || !accessUnit->meta()->findInt64("timeUs", &int64Value)) {
> +        return;
> +    }

Use assertion MOZ_ASSERT() as well.

Comment 15

[Jonathan Hao (inactive) [:jhao]]

Attachment: bug-1045062-fix.patch

Thanks to Steve and Ethan for previous review. I've fixed the parts you mentioned.

I have discussed with Ethan about the error handling where the attributes "session", "transport" are missing in headers. The attributes are required by the protocol in RFC document, so we'll keep using assertions on them for now.

Note that sometimes CHECK cannot be simply replaced by MOZ_ASSERT, because when the DEBUG symbol is off, MOZ_ASSERT is rewritten to "do {} while(0);". A line like:
> MOZ_ASSERT(accessUnit->meta()->findInt64("timeUs", &int64Value));
may not read the in64Value at all. That's why I used:
> bool success = accessUnit->meta()->findInt64("timeUs", &int64Value);
> MOZ_ASSERT(success);

Comment 16

[Ethan Tseng [:ethan]]

(In reply to Jonathan Hao [:jhao] from comment #15)
> Note that sometimes CHECK cannot be simply replaced by MOZ_ASSERT, because
> when the DEBUG symbol is off, MOZ_ASSERT is rewritten to "do {} while(0);".
> A line like:
> > MOZ_ASSERT(accessUnit->meta()->findInt64("timeUs", &int64Value));
> may not read the in64Value at all. That's why I used:
> > bool success = accessUnit->meta()->findInt64("timeUs", &int64Value);
> > MOZ_ASSERT(success);

You are right! We should be aware of this.
Thus, we should not write any code with side effects within MOZ_ASSERT().

BTW, you can build a DEBUG version by adding the following line to b2g/.userconfig:
export B2G_DEBUG=1

In order to avoid unnecessary re-build time, you can tweak .userconfig by adjusting GECKO_OBJDIR
according to the value of B2G_DEBUG.

Comment 17

[Ethan Tseng [:ethan]]

Comment on attachment 8500356 [details] [diff] [review]
bug-1045062-fix.patch

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

The patch looks fine - some cleanups needed.
Meanwhile, you might need to rebase the file RTSPSource.cpp since it was changed recently.

::: netwerk/protocol/rtsp/rtsp/AMPEG4AudioAssembler.cpp
@@ +577,5 @@
>  
> +bool AMPEG4AudioAssembler::submitAccessUnit() {
> +    if (mPackets.empty()) {
> +        return false;
> +    }

Use MOZ_ASSERT().

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +779,5 @@
>                          static_cast<ARTSPResponse *>(obj.get());
>                      if (response->mStatusCode != 200) {
>                          result = UNKNOWN_ERROR;
> +                    // } else if (!parsePlayResponse(response)) {
> +                    //     result = UNKNOWN_ERROR;

Nit. Please remove these two lines if they're not used.

@@ +784,2 @@
>                      } else {
>                          parsePlayResponse(response);

You changed the return type of parsePlayResponse().
I think you plan to check its return value, don't you?

@@ +1149,5 @@
>  
>                      if (response->mStatusCode != 200) {
>                          result = UNKNOWN_ERROR;
> +                    // } else if (!parsePlayResponse(response)) {
> +                    //     result = UNKNOWN_ERROR;

Ditto.

@@ +1154,2 @@
>                      } else {
>                          parsePlayResponse(response);

The same question as above.

Comment 18

[Jonathan Hao (inactive) [:jhao]]

Attachment: bug-1045062-rev.patch

You're right. I was going to check the return value, but somehow left it commented. Now it is

> if (response->mStatusCode != 200) {
>     result = UNKNOWN_ERROR;
> } else if (!parsePlayResponse(response)) {
>     result = UNKNOWN_ERROR;
> } else {

Thanks again.

Comment 19

[Ethan Tseng [:ethan]]

Comment on attachment 8505224 [details] [diff] [review]
bug-1045062-rev.patch

(In reply to Steve Workman [:sworkman] from comment #13)
> In any case, I think Ethan knows RTSP better than I do and will know how
> each error should be handled. So, I'm going to delegate the review to him.
> I'll take a quick look once he thinks it's ok.

Hi Steve,

The patch is sane to me.
Please help to do a quick review, thanks. :)

Comment 20

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8505224 [details] [diff] [review]
bug-1045062-rev.patch

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

Good progress! Some comments so keeping the review open.

::: netwerk/protocol/rtsp/rtsp/ARTPSession.cpp
@@ +158,5 @@
>                  break;
>              }
>  
>              sp<RefBase> obj;
>              CHECK(msg->findObject("access-unit", &obj));

Did you want to remove this CHECK?

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +609,1 @@
>              meta->SetSampleRate(int32Value);

This will crash in debug code, but in release we will set whatever value is in int32Value. Is that ok?

::: netwerk/protocol/rtsp/rtsp/RTSPTransmitter.h
@@ -389,5 @@
>                              break;
>                          }
> -
> -                        authenticate(response);
> -                        break;

Looks like this break is now removed completely, so 401 status codes will only break if there's no auth type and the response can't be authenticated. Is this what you want to do?

@@ +466,5 @@
>                  LOG(INFO) << "SETUP completed with result "
>                       << result << " (" << strerror(-result) << ")";
>  
>                  sp<RefBase> obj;
>                  CHECK(msg->findObject("response", &obj));

There are a couple of CHECK statements still here. Do you want to keep them? Are they for another patch?

::: netwerk/protocol/rtsp/rtsp/UDPPusher.cpp
@@ +100,5 @@
>      ssize_t n = PR_SendTo(
>              mSocket, buffer->data(), buffer->size(), 0,
>              &mRemoteAddr, PR_INTERVAL_NO_WAIT);
>  
>      CHECK_EQ(n, (ssize_t)buffer->size());

Did you want to remove this CHECK_EQ?

Comment 21

[Jonathan Hao (inactive) [:jhao]]

Thanks again to Steve.

About "CHECK(msg->findXXX("xxx", ...))", Ethan and I think that since the attributes of the messages must be set by "new AMessage("xxx")" somewhere in our code, the assertion should never fail unless there're logic mistakes, so they aren't replaced by error-handling. However, they could be replaced with MOZ_ASSERT if needed, perhaps in another patch or another bug?

> ::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
> @@ +609,1 @@
> >              meta->SetSampleRate(int32Value);
> 
> This will crash in debug code, but in release we will set whatever value is
> in int32Value. Is that ok?

Yes, that's the reason why we're changing CHECK to MOZ_ASSERT, but Ethan did point out that we haven't been testing debug build, manually or automatically. We should test debug build more often.

> ::: netwerk/protocol/rtsp/rtsp/RTSPTransmitter.h
> @@ -389,5 @@
> >                              break;
> >                          }
> > -
> > -                        authenticate(response);
> > -                        break;
> Looks like this break is now removed completely, so 401 status codes will only break if there's no
> auth type and the response can't be authenticated. Is this what you want to do?

This one was my mistake. The "break" shouldn't be removed.

> 
> ::: netwerk/protocol/rtsp/rtsp/UDPPusher.cpp
> @@ +100,5 @@
> >      ssize_t n = PR_SendTo(
> >              mSocket, buffer->data(), buffer->size(), 0,
> >              &mRemoteAddr, PR_INTERVAL_NO_WAIT);
> >  
> >      CHECK_EQ(n, (ssize_t)buffer->size());
> 
> Did you want to remove this CHECK_EQ?

Yes, this can be replaced with MOZ_ASSERT.

Comment 22

[Jonathan Hao (inactive) [:jhao]]

Attachment: bug-1045062-rev2.patch

This patch fix the mistake in the code of authentication Steve pointed out.

Comment 23

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8505991 [details] [diff] [review]
bug-1045062-rev2.patch

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

(In reply to Jonathan Hao [:jhao] from comment #21)
> Thanks again to Steve.
You're welcome :)

> About "CHECK(msg->findXXX("xxx", ...))", Ethan and I think that since the
> attributes of the messages must be set by "new AMessage("xxx")" somewhere in
> our code, the assertion should never fail unless there're logic mistakes, so
> they aren't replaced by error-handling. However, they could be replaced with
> MOZ_ASSERT if needed, perhaps in another patch or another bug?

Sounds good.

> Yes, that's the reason why we're changing CHECK to MOZ_ASSERT, but Ethan did
> point out that we haven't been testing debug build, manually or
> automatically. We should test debug build more often.

Understood. Once automated testing is figured out this code path will be executed more. For general use, I suggest "ac_add_options" --enable-debug and "ac_add_options --enable-optimize". It might be more difficult for gdb, but it should enable all the MOZ_ASSERTs.

r=me - thanks!

Comment 24

[Jonathan Hao (inactive) [:jhao]]

Attachment: bug-1045062-fix.patch

Adding "r=sworkman" to commit message.

Comment 25

[Jonathan Hao (inactive) [:jhao]]

Try server results:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8c2cae76df5b

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ad7a09962cc4

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ad7a09962cc4

Comment 28

[Ethan Tseng [:ethan]]

Attachment: bug-1045062-fix-2.1.patch

This patch is in order to uplift to v2.1.
It is required to resolve the system crash issue in bug 1110663.

Comment 29

[Ethan Tseng [:ethan]]

Comment on attachment 8536496 [details] [diff] [review]
bug-1045062-fix-2.1.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 30

[Ethan Tseng [:ethan]]

Comment on attachment 8536496 [details] [diff] [review]
bug-1045062-fix-2.1.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1110663
User impact if declined: System crash
Testing completed: On v2.1
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

Comment 31

[Ethan Tseng [:ethan]]

The result of Treeherder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7ce0c98f0246

Android 2.3/4.0 platform build busted.
But according to the error logs I think it's the build server issue.

Comment 32

[Ethan Tseng [:ethan]]

Hi Bhavana,

Our partner needs this patch in v2.1 in order to avoid a system crash issue.
Could you help to grant the uplift request?

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f2744aacd97c