Closed Bug 1346235 Opened 3 years ago Closed 3 years ago

Fennec has stopped working while playing videos

Categories

(Firefox for Android :: Audio/Video, defect, P1)

55 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
fennec + ---
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- verified
firefox55 --- verified

People

(Reporter: sflorean, Assigned: jhlin)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Environment: 
Device:
- Samsung Galaxy Note 4 (Android 5.0.1);
- Motorola Nexus 6 (Android 7.0)
Build: Nightly 55.0a1 (2017-03-10);

Steps to reproduce:
1. Open Fennec with a clean profile;
2. Go to youtube and play a video;


Expected result:
The video is playing with no issues.

Actual result:
After more than 10 sec the message is displayed "Unfortunately, Aurora/Nightly has stopped."
Attached file log.txt
See Also: → 1326026
tracking-fennec: --- → ?
Hi Blake,

Is this bug related to OOP?  May we please ask John to take a look ?

Besides, 

From the screenshot, I am wondering if it has anything to do with shutdown decoder?
However, since it happened on both aurora54 and Nightly55, I suppose this bug should have nothing to do with shutdown decoder.

Can you please help us clarify the issue ? Thank you very much !
Flags: needinfo?(bwu)
This should result from the crash in out-of-process, OOP. 
Sorina, 
Do you hit this problem often? what is the repro rate?
Flags: needinfo?(bwu) → needinfo?(sorina.florean)
(In reply to Blake Wu [:bwu][:blakewu] from comment #3)
> This should result from the crash in out-of-process, OOP. 
> Sorina, 
> Do you hit this problem often? what is the repro rate?

Hello, tested again with the devices from description and the repro rate is 3/4 and I've seen this problem on different sites like cnn.com, vimeo not just youtube. Let me know if is other info needed.
Flags: needinfo?(sorina.florean)
Hello,

 On Xiaomi Mi Pad 2 (5.1 - x86) the videos stop playing when accessing certain news site like cnn.com or nexs.google.com and doesn't resume playback. I think the issue might be related to this since on Nexus 6 (7.0) I get the same result when accessing a news article from news.google.com and I also received the error message mentioned by Sorina when testing with the Nexus on Aurora.

 This only seems to happen when accessing a news site from what I've seen so far, works fine when going on other pages (eg. Amazon, eBay, Facebook... ).

STR:
 1. Go to youtube.com (redirected to m.youtube.com) and play any video;
 2. Go to cnn.com or news.google.com (on news google open any news article);
 3. Wait for the page to load.

Expected result:
 Video playback should not pause while the page is loading or finishes loading.

Actual results:
 - The video playback pauses and does not resume on the Xiaomi Mi Pad 2 (5.1 - x86);
 - The video playback pauses and resumes after several seconds on Nexus 6 (7.0).

Notes:
 The issue seems to not reproduce on news sites like foxnews, nbcnews, usatoday...
Since this issue is preventing me from testing how suspended video decoder elements perform after waiting for a longer period of time I shall mark it as a blocker for the main feature.
John, 
Would this be the same to bug 1345599?
Assignee: nobody → jolin
Flags: needinfo?(jolin)
Priority: -- → P1
(In reply to Blake Wu [:bwu][:blakewu] from comment #7)
> John, 
> Would this be the same to bug 1345599?

 Looks like it. Patches in bug 1345599 should eliminate the out of memory situation that causes the crashes.
 I'll add proper error handling code in this bug to prevent crashing when shared memory fails.
Flags: needinfo?(jolin)
Comment on attachment 8848480 [details]
Bug 1346235 - part 1: translate native error to Java exception.

https://reviewboard.mozilla.org/r/121408/#review123586

::: mobile/android/base/java/org/mozilla/gecko/mozglue/SharedMemory.java:130
(Diff revision 1)
>  
>      private int getFD() {
>          return isValid() ? mDescriptor.getFd() : -1;
>      }
>  
> -    public long getPointer() {
> +    public long getPointer() throws NullPointerException {

Don't need this change.

::: mobile/android/base/java/org/mozilla/gecko/mozglue/SharedMemory.java:149
(Diff revision 1)
>              }
>          }
>          return mHandle;
>      }
>  
> -    private native long map(int fd, int size);
> +    private native long map(int fd, int size) throws NullPointerException;

Don't need this change.

::: mozglue/android/SharedMemNatives.cpp:57
(Diff revision 1)
>  JNIEXPORT
>  jlong JNICALL
>  Java_org_mozilla_gecko_mozglue_SharedMemory_map(JNIEnv *env, jobject jobj, jint fd, jint length)
>  {
>    void* address = mmap(NULL, length, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> +    if (address == MAP_FAILED) {

Fix alignment
Attachment #8848480 - Flags: review?(nchen) → review+
Comment on attachment 8848481 [details]
Bug 1346235 - part 2: translate memory error to I/O failure.

https://reviewboard.mozilla.org/r/121410/#review123588

::: mobile/android/base/java/org/mozilla/gecko/mozglue/SharedMemBuffer.java:60
(Diff revision 1)
>              throw new IOException("SharedMemBuffer only support reading from direct byte buffer.");
>          }
> +        try {
> -        nativeReadFromDirectBuffer(src, mSharedMem.getPointer(), offset, size);
> +            nativeReadFromDirectBuffer(src, mSharedMem.getPointer(), offset, size);
> +        } catch (NullPointerException e) {
> +            throw new IOException(e.getMessage());

throw new IOException(e);

::: mobile/android/base/java/org/mozilla/gecko/mozglue/SharedMemBuffer.java:74
(Diff revision 1)
>              throw new IOException("SharedMemBuffer only support writing to direct byte buffer.");
>          }
> +        try {
> -        nativeWriteToDirectBuffer(mSharedMem.getPointer(), dest, offset, size);
> +            nativeWriteToDirectBuffer(mSharedMem.getPointer(), dest, offset, size);
> +        } catch (NullPointerException e) {
> +            throw new IOException(e.getMessage());

throw new IOException(e);
Attachment #8848481 - Flags: review?(nchen) → review+
Comment on attachment 8848482 [details]
Bug 1346235 - part 3: forward shared memory allocation error to peer process.

https://reviewboard.mozilla.org/r/121412/#review123592

::: mobile/android/base/java/org/mozilla/gecko/media/SamplePool.java:47
(Diff revision 1)
>              } else {
>                  return allocateSharedMemorySample(size);
>              }
>          }
>  
> -        private Sample allocateSharedMemorySample(int size) {
> +        private Sample allocateSharedMemorySample(int size) throws NullPointerException {

Don't need this change.

::: mobile/android/base/java/org/mozilla/gecko/media/SamplePool.java:52
(Diff revision 1)
> -        private Sample allocateSharedMemorySample(int size) {
> +        private Sample allocateSharedMemorySample(int size) throws NullPointerException {
>              SharedMemory shm = null;
>              try {
>                  shm = new SharedMemory(mNextId++, Math.max(size, mDefaultBufferSize));
> -            } catch (NoSuchMethodException e) {
> -                e.printStackTrace();
> +            } catch (NoSuchMethodException | IOException e) {
> +                throw new NullPointerException(e.getMessage());

I don't think `NullPointerException` is the appropriate exception here. Maybe `UnsupportedOperationException`?

    throw new UnsupportedOperationException(e);
Attachment #8848482 - Flags: review?(nchen) → review+
Comment on attachment 8848483 [details]
Bug 1346235 - part 4: recycle unpopulated input samples.

https://reviewboard.mozilla.org/r/121414/#review123594

::: mobile/android/base/java/org/mozilla/gecko/media/CodecProxy.java:196
(Diff revision 1)
> -            }
> +    }
> +
> +    private boolean sendInput(Sample sample) {
> +        try {
>              mRemote.queueInput(sample);
>              sample.dispose();

`sample` can be null here (e.g. when you call `sendInput(null)` above)
Attachment #8848483 - Flags: review?(nchen) → review+
Pushed by jolin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa8084f4f200
part 1: translate native error to Java exception. r=jchen
https://hg.mozilla.org/integration/autoland/rev/639a9dcab2bb
part 2: translate memory error to I/O failure. r=jchen
https://hg.mozilla.org/integration/autoland/rev/0dad897e64bd
part 3: forward shared memory allocation error to peer process. r=jchen
https://hg.mozilla.org/integration/autoland/rev/c4f7d0039351
part 4: recycle unpopulated input samples. r=jchen
Sorina,
Please help us test it again.
Thank you.
Flags: needinfo?(sorina.florean)
Hi all, 

Tested with:
- Samsung Galaxy Note 4 (Android 5.0.1);
- Xiaomi Mi Pad 2 (Android 5.1 x86);
- Nexus 6 (Android 7.0);
- Lenovo A536 (Android 4.4.2).

This bug seems fixed, the videos are playing with no issues. As what Bogdan wrote in comment #5 and following the steps from description, the video is not paused and continue playing while another tab with news is loaded. So that issue is fixed too.
Flags: needinfo?(sorina.florean)
tracking-fennec: ? → +
Verified as fixed on Beta build 54.0b2;
Device: Nexus 6 (Android 7.0).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.