922334 - crash in sysconf [B2G][Browser][Youtube] Playing youtube videos crash the browser tab

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[gbennett]

Attachment: YoutubeCrashLOG.txt

This bug was filed from the Socorro interface and is 
report bp-2c4cabe2-5bd4-4494-9bd2-0d8b62130930.
=============================================================
Description:
After launching browser and tapping to play a youtube video the loading wheel freeses and the browser tab crashes with a user friendly error message.

Repro Steps:
1) Updated Buri 1.3 mozRIL to Build ID: 20130930071202
2) Open browser app
3) Go to youtube.com
4) Click and play any video

Actual:
Youtube browser tab crashes.

Expected:
Youtube does not crash and video plays properly.

Environmental Variables
Device: Buri 1.3 mozRIL
Build ID: 20130930071202
Gecko: http://hg.mozilla.org/mozilla-central/rev/5a49762ee832
Gaia: e7c011371aa6af696033d4b867fb9152a6985efa
Platform Version: 27.0a1
Firmware Version: 20130912

Repro frequency: 100%, 8/8
Test Suite Name: Firefox OS Daily Smoketest
UCID: N/A
Link to failed test case: https://moztrap.mozilla.org/manage/case/6073/
See attached: YoutubeCrashLOG.txt

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

reseting the phone causes the crash to not appear.  It appears that there may be some residue data that's causing the crashing to occur.  To note, when I didn't reset the phone I also did see the crash.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I was mistaken.  This happen even with the reset, I hit it without going to youtube:

https://crash-stats.mozilla.com/report/index/48493bd8-6012-4164-9a1e-b59ba2131001

Comment 3

[Jason Smith [:jsmith]]

Analyzing the push log, the only patch candidates that landed since 9/27 that look related to this crash were implemented by kinetik.

Comment 4

[Sotaro Ikeda [:sotaro]]

From the investigation by using git log, the following change seems make the regression.

-----------------------------------------------------------
commit 5ce2711c7a26f148c840ccdfa8bd4c239d4b4fc5
Author: Nathan Froyd <[e-mail]>
Date:   Tue Sep 10 15:55:39 2013 -0400

    Bug 914826 - part 6 - don't include basictypes.h or nscore.h in ipdl headers; r=ehsan

Comment 5

[Sotaro Ikeda [:sotaro]]

I narrow downed the bug until around Bug 914826. The following the result of the youtube playback check result and related git log.

-------------------------------------------------------------------
commit dda25a1f898331a454f1983e24ac8381ea761bab < youtube failed ************************************>
Author: Andrew McCreight <[e-mail]>
Date:   Thu Sep 26 16:46:42 2013 -0700

    Bug 920840 - Crash when JS-implemented WebIDL's init method returns a value other than undefined. r=bz

commit 5ce2711c7a26f148c840ccdfa8bd4c239d4b4fc5 < youtube failed ************************************>
Author: Nathan Froyd <[e-mail]>
Date:   Tue Sep 10 15:55:39 2013 -0400

    Bug 914826 - part 6 - don't include basictypes.h or nscore.h in ipdl headers; r=ehsan

commit 12fa756043d7c69cbacc8dc17f253d8607017280 < youtube works ************************************>
Author: Nathan Froyd <[e-mail]>
Date:   Tue Sep 10 15:45:16 2013 -0400

    Bug 914826 - part 5 - provide for cpp-only include files, starting with nsIFile.h and GeckoProfiler.h; r=ehsan

commit e8316ed759446e66efd6bf727ddc2b287b140d4d
Author: Nathan Froyd <[e-mail]>
Date:   Tue Sep 10 16:56:05 2013 -0400

    Bug 914826 - part 4 - fix source files that were bootlegging XPCOM do_* functions via generated ipdl headers; r=ehsan

commit 44d376b745ac49dff7c492c44d6eddabbc90004c
Author: Nathan Froyd <[e-mail]>
Date:   Tue Sep 10 15:35:51 2013 -0400

    Bug 914826 - part 3 - forward-declare nsIFile for GetMinidump() declaration; r=ehsan

commit a4908f9bf19c9800966ec4afeafea98ab71d294b
Author: Nathan Froyd <[e-mail]>
Date:   Tue Sep 10 15:34:44 2013 -0400

    Bug 914826 - part 2 - rename builtinIncludes to builtinHeaderIncludes; r=ehsan

commit 515c39f6fad0cc10afd0a338656ca195a056f6c6
Author: Nathan Froyd <[e-mail]>
Date:   Tue Sep 10 15:07:10 2013 -0400

    Bug 914826 - part 1 - use static_assert instead of COMPILE_ASSERT in IPCMessageStart.h; r=ehsan

commit 8aa87ed7e6b980179dad08ce520c7c1654145508  < youtube works ****************************************>
Author: Eitan Isaacson <[e-mail]>
Date:   Fri Sep 27 10:03:05 2013 -0700

    Bug 918388 - Enable Synth Web Speech API pref in b2g. r=fabrice r=smaug

Comment 6

[Sotaro Ikeda [:sotaro]]

Just reverting only the following from recent code did not fix the problem.
- Bug 914826 - part 6 - don't include basictypes.h or nscore.h in ipdl headers; r=ehsan

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> Just reverting only the following from recent code did not fix the problem.
> - Bug 914826 - part 6 - don't include basictypes.h or nscore.h in ipdl
> headers; r=ehsan

Looks like bug 920840 is the problem, then, no?

Comment 8

[Andrew McCreight [:mccr8]]

I see this in the log:
09-30 13:37:53.020: I/Gecko(3409): No permission to use the keyboard API for http://m.youtube.com
09-30 13:37:53.200: I/Gecko(3409): No permission to use the keyboard API for http://m.youtube.com

It looks like the page is trying to use a privileged API (?!?) which now fails more aggressively due to a combination of bug 920831 and bug 920840.  I think bug 920831 is a more likely regressor, given that it landed on m-c on 9-30, and bug 920840 hasn't landed on m-c yet.  But I'd guess the real problem here is whatever the page is doing that is trying to use the keyboard without permissions.

Comment 9

[Andrew McCreight [:mccr8]]

I doubt that YouTube's mobile site is attempting to use privileged B2G APIs, so I'd guess this is a Gaia problem somehow.

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Nathan Froyd (:froydnj) from comment #7)
> (In reply to Sotaro Ikeda [:sotaro] from comment #6)
> > Just reverting only the following from recent code did not fix the problem.
> > - Bug 914826 - part 6 - don't include basictypes.h or nscore.h in ipdl
> > headers; r=ehsan
> 
> Looks like bug 920840 is the problem, then, no?

I am going to re-check.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Sotaro is quite busy these days - can we get somebody else to find the regression range, especially since it seems to be outside the graphics work?  I can't actually tell, I don't have access to either of the two bugs mentioned as possible culprits.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #11)
> Sotaro is quite busy these days - can we get somebody else to find the
> regression range, especially since it seems to be outside the graphics work?
> I can't actually tell, I don't have access to either of the two bugs
> mentioned as possible culprits.

Not suggesting others aren't busy - just that Sotaro is on a couple of critical paths, and it'd be good if he wasn't on one more.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: stackinfo of the crash

When the crash happen, stack overflow seems to happen.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Sotaro Ikeda [:sotaro] from comment #13)
> Created attachment 812649 [details]
> stackinfo of the crash
> 
> When the crash happen, stack overflow seems to happen.

Having the full stack and not just a truncated stack would be good (keep hitting <Enter> in GDB until you get back to a (gdb) prompt).

But since this is infinite recursion in the runtime, I'm going to guess that somebody's doing a division that has undefined results.  Providing a full stack will point the finger at who's doing that.

Comment 15

[Andrew McCreight [:mccr8]]

If this doesn't reproduce with the dom.mozInputMethod.testing pref set to true, then that suggests bug 920831 isn't at fault.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: stackinfo of the crash on nexus4

On nexus4, I could get meaningful stacktrace.

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: original regression rage of the git log

At first, I got a change list by uisng git log. I used it to check which change made regression.

Comment 18

[Sotaro Ikeda [:sotaro]]

In attachment 812706 [details], there is a change around mp3.

----------------------------------------------------
commit 9772d4c7cfb0801219b16c780d4caf73b4404394
Author: Edwin Flores <[e-mail]>
Date:   Sat Sep 28 16:33:32 2013 +1200

    Bug 919572 - Refactor the MP3 frame parser r=cpearce

Comment 19

[Sotaro Ikeda [:sotaro]]

It is not clear why master hamachi did not crash around the change in Comment 18.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Does the patch from bug 920831 fix this?

Comment 21

[Andrew McCreight [:mccr8]]

Sorry, this bug should block that bug, on the theory that it is a regression from it. Comment 15 is one way we could test if it really is or not. (Also, I've cc'ed you on it, Milan.)

Comment 22

[Jason Smith [:jsmith]]

(In reply to Andrew McCreight [:mccr8] from comment #21)
> Sorry, this bug should block that bug, on the theory that it is a regression
> from it. Comment 15 is one way we could test if it really is or not. (Also,
> I've cc'ed you on it, Milan.)

I don't think that's right. bug 902831 landed on early afternoon on 9/30, but this bug was filed on a build before that patch landed.

Comment 23

[Sotaro Ikeda [:sotaro]]

Following code does 0 division when the file is not mp3. It is also a problem that MP3FrameParser is called even when the file is not mp3. By changing the code locally, I confirmed the crash is fixed. And the mp3's change also causes Bug 912829.

-----------------------------------------------------------------
>   // The duration of each frame is constant over a given stream.
>   double usPerFrame = USECS_PER_S * SAMPLES_PER_FRAME / mSampleRate;

http://mxr.mozilla.org/mozilla-central/source/content/media/MP3FrameParser.cpp#377

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> Following code does 0 division when the file is not mp3. It is also a
> problem that MP3FrameParser is called even when the file is not mp3. By
> changing the code locally, I confirmed the crash is fixed. And the mp3's
> change also causes Bug 912829.

Bug 919572 also causes youtube playback failure. But it seems different one from Bug 912829.

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add zero division check

The patch fixes the crash, but youtube still can not be playbacked. mp3 parser doing parsing even when youtube is not mp3.

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add zero division check and prevent unncecessary mp3 parsing

Comment 27

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #26)
> Created attachment 812799 [details] [diff] [review]
> patch - Add zero division check and prevent unncecessary mp3 parsing

By the patch, youtube can be playbacked on master hamachi.

Comment 28

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=2316e1ec9342

Comment 29

[Sotaro Ikeda [:sotaro]]

Comment on attachment 812799 [details] [diff] [review]
patch - Add zero division check and prevent unncecessary mp3 parsing

doublec, can you review the patch soon? This bug is top crash. Thanks.

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Add zero division check and prevent unncecessary mp3 parsing

Commitable patch. Carry "chris.double: review+".

Comment 31

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec8fcf012bd

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2ec8fcf012bd

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 for b2gv1.2 - Add zero division check and prevent unncecessary mp3 parsing

Committable patch for b2g v1.2.

Comment 34

[Sotaro Ikeda [:sotaro]]

This bug blocks ko1+ Bug 919596. Need to uplift of b2g v1.2.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/74e15e692c0d