979761 - [RTSP] Crash when playing RTSP streaming with MPEG AAC Audio codec (regression caused by bug 970271)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Ethan Tseng [:ethan]]

Attachment: crash_at_AMPEG4AudioAssembler.log

When we play an RTSP media which uses codec "MPEG AAC Audio (mp4a)", the system crash would crash immediately.
The gdb backtrace log is attached.

This is a regression caused by the changeset b5d404be0b1e from bug 970271.
The changeset fixed GCC warnings by removing some unused variables, and the root cause is the removal of this line of code:
unsigned frameLengthFlag = bits->getBits(1);

ABitReader::getBits() has side effect. It would change the values of its internal data members related to bit positions.
Thus it causes parsing mistake by removing that line of code.

Comment 1

[Ethan Tseng [:ethan]]

Nominate this bug as a 1.4 blocker since the impact is critical.

Comment 2

[Ethan Tseng [:ethan]]

Attachment: bug-979761-fix.patch

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8385939 [details] [diff] [review]
bug-979761-fix.patch

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

This can be done with

#include <mozilla/unused.h>

unused << bits->getBit(1);

like in the patch at  https://bug970271.bugzilla.mozilla.org/attachment.cgi?id=8380729

That should fix the bug, but not produce a warning from gcc.

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> Comment on attachment 8385939 [details] [diff] [review]
> bug-979761-fix.patch
> 
> Review of attachment 8385939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This can be done with
> 
> #include <mozilla/unused.h>
> 
> unused << bits->getBit(1);
> 
> like in the patch at 
> https://bug970271.bugzilla.mozilla.org/attachment.cgi?id=8380729
> 
> That should fix the bug, but not produce a warning from gcc.

Sorry, forget about that comment. I was somehow thinking of a different warning.

Comment 5

[Ethan Tseng [:ethan]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4)
> Sorry, forget about that comment. I was somehow thinking of a different
> warning.
Never mind. Thanks for sharing the unused tool.
Actually I tried to fix that warning (ignoring return value) few days ago, but didn't have a good idea.
Glad to learn this trick from your patch. :)

Comment 6

[Ethan Tseng [:ethan]]

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

Comment 7

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8385939 [details] [diff] [review]
bug-979761-fix.patch

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

To prevent confusing people on what getBits() is doing, I think we should add more comments to explain in more detail. 
Crash is really critical, so let's clean this file up in the follow up.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Vincent Chang[:vchang] from comment #7)
> Comment on attachment 8385939 [details] [diff] [review]
> bug-979761-fix.patch
> 
> Review of attachment 8385939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> To prevent confusing people on what getBits() is doing, I think we should
> add more comments to explain in more detail. 
> Crash is really critical, so let's clean this file up in the follow up.

I totally agree. I suggest to rename |getBits| to indicate that it has side effects.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ethan Tseng [:ethan]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> I totally agree. I suggest to rename |getBits| to indicate that it has side
> effects.
Yes, we also thought of renaming this function to reflect its side effects in the first place.
Unfortunately, this source file is in AOSP's foundation, not in our repository.
They are in:
  B2G/frameworks/base/media/libstagefright/foundation/ABitReader.cpp
  B2G/frameworks/base/include/media/stagefright/foundation/ABitReader.h

That's why we couldn't simply refine the method name.
I filed bug 980226 to follow up this issue according to Vincent's advice.
We will refactor some codes which invoke getBits() to avoid future misuse.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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