Sorina Florean [:sflorean] Reporter
	Description • 7 years ago

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."

	Rachelle Yang [:ryang][ryang@mozilla.com]
	Comment 2 • 7 years ago

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 !

	Blake Wu [:bwu][:blakewu]
	Comment 3 • 7 years ago

This should result from the crash in out-of-process, OOP. 
Sorina, 
Do you hit this problem often? what is the repro rate?

	Sorina Florean [:sflorean] Reporter
	Comment 4 • 7 years ago

(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.

	Bogdan Surd, QA [:bsurd, NI]
	Comment 5 • 7 years ago

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...

	Bogdan Surd, QA [:bsurd, NI]
	Comment 6 • 7 years ago

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.

	Blake Wu [:bwu][:blakewu]
	Comment 7 • 7 years ago

John, 
Would this be the same to bug 1345599?

	John Lin [:jhlin][:jolin] Assignee
	Comment 8 • 7 years ago

(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.

	(inactive) Jim Chen [:jchen] [:darchons]
	Comment 13 • 7 years ago
mozreview-review

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

	(inactive) Jim Chen [:jchen] [:darchons]
	Comment 14 • 7 years ago
mozreview-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);

	(inactive) Jim Chen [:jchen] [:darchons]
	Comment 15 • 7 years ago
mozreview-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);

	(inactive) Jim Chen [:jchen] [:darchons]
	Comment 16 • 7 years ago
mozreview-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)

	Pulsebot
	Comment 25 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 26 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fa8084f4f200
https://hg.mozilla.org/mozilla-central/rev/639a9dcab2bb
https://hg.mozilla.org/mozilla-central/rev/0dad897e64bd
https://hg.mozilla.org/mozilla-central/rev/c4f7d0039351

	Blake Wu [:bwu][:blakewu]
	Comment 27 • 7 years ago

Sorina,
Please help us test it again.
Thank you.

	Sorina Florean [:sflorean] Reporter
	Comment 28 • 7 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/fb9caf2d4e40
https://hg.mozilla.org/releases/mozilla-aurora/rev/14fd7f066eb9
https://hg.mozilla.org/releases/mozilla-aurora/rev/6e00d34c16e7
https://hg.mozilla.org/releases/mozilla-aurora/rev/8130607e5dbf

	Bogdan Surd, QA [:bsurd, NI]
	Comment 30 • 7 years ago

Verified as fixed on Beta build 54.0b2;
Device: Nexus 6 (Android 7.0).