824294 - crash in nsHTMLMediaElement::AddRemoveSelfReference

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Michael Vines [:m1] [:evilmachines]]

Crash seen multiple times during stability test.

Crash reason:  SIGSEGV
Crash address: 0xc

Thread 0 (crashed)
 0  libxul.so!nsHTMLMediaElement::AddRemoveSelfReference [nsINodeInfo.h : 279 + 0x0]
     r4 = 0x00000000    r5 = 0xfffffff4    r6 = 0xffffffff    r7 = 0x00000000
     r8 = 0x43235f9c    r9 = 0x41a06bac   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec90ee8    lr = 0x40740ae9    pc = 0x4073e15e
    Found by: given as instruction pointer in context
 1  libxul.so!nsHTMLMediaElement::PlaybackEnded [nsHTMLMediaElement.cpp : 3056 + 0x3]
     r4 = 0x00000000    r5 = 0xfffffff4    r6 = 0xffffffff    r7 = 0x00000000
     r8 = 0x43235f9c    r9 = 0x41a06bac   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec90f08    pc = 0x40740ae9
    Found by: call frame info
 2  libxul.so!nsBuiltinDecoder::PlaybackEnded [nsBuiltinDecoder.cpp : 716 + 0x5]
     r4 = 0x43235f00    r5 = 0xfffffff4    r6 = 0xffffffff    r7 = 0x43235fa0
     r8 = 0x43235f9c    r9 = 0x41a06bac   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec90f38    pc = 0x408d02d1
    Found by: call frame info
 3  libxul.so!nsRunnableMethodImpl<nsrefcnt (mozilla::dom::workers::DOMBindingBase::*)(), false>::Run [nsThreadUtils.h : 349 + 0x5]
     r4 = 0x41a06b80    r5 = 0x00000000    r6 = 0x00000000    r7 = 0x00000001
     r8 = 0xbec90fa7    r9 = 0x41a06bac   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec90f58    pc = 0x404614bd
    Found by: call frame info
 4  libxul.so!nsThread::ProcessNextEvent [nsThread.cpp : 620 + 0x5]
     r4 = 0x41a06b80    r5 = 0x00000000    r6 = 0x00000000    r7 = 0x00000001
     r8 = 0xbec90fa7    r9 = 0x41a06bac   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec90f60    pc = 0x40c1ac9b
    Found by: call frame info
 5  libxul.so!NS_ProcessNextEvent_P [nsThreadUtils.cpp : 220 + 0xb]
     r4 = 0x00000000    r5 = 0xbec918b8    r6 = 0x41a022f0    r7 = 0x00000001
     r8 = 0x41a23000    r9 = 0x41a28000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec90fa0    pc = 0x40bfb0b7
    Found by: call frame info
 6  libxul.so!mozilla::ipc::MessagePump::Run [MessagePump.cpp : 82 + 0x7]
     r4 = 0x41a022e0    r5 = 0xbec918b8    r6 = 0x41a022f0    r7 = 0x00000001
     r8 = 0x41a23000    r9 = 0x41a28000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec90fb0    pc = 0x40b32d35
    Found by: call frame info
 7  libxul.so!mozilla::ipc::MessagePumpForChildProcess::Run [MessagePump.cpp : 231 + 0x7]
     r4 = 0xbec918b8    r5 = 0x41a022e0    r6 = 0xbec918b8    r7 = 0x00000001
     r8 = 0x41a23000    r9 = 0x41a28000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec90fd8    pc = 0x40b32de7
    Found by: call frame info
 8  libxul.so!MessageLoop::RunInternal [message_loop.cc : 215 + 0x5]
     r4 = 0xbec918b8    r5 = 0x426cbf40    r6 = 0x41a06b80    r7 = 0x00000003
     r8 = 0x41a23000    r9 = 0x41a28000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec90ff0    pc = 0x40c3c409
    Found by: call frame info
 9  libxul.so!MessageLoop::Run [message_loop.cc : 208 + 0x5]
     r4 = 0xbec918b8    r5 = 0x426cbf40    r6 = 0x41a06b80    r7 = 0x00000003
     r8 = 0x41a23000    r9 = 0x41a28000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec90ff8    pc = 0x40c3c4bf
    Found by: call frame info
10  libxul.so!nsBaseAppShell::Run [nsBaseAppShell.cpp : 163 + 0x7]
     r4 = 0x00000000    r5 = 0x426cbf40    r6 = 0x41a06b80    r7 = 0x00000003
     r8 = 0x41a23000    r9 = 0x41a28000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec91010    pc = 0x40abb751
    Found by: call frame info
11  libxul.so!XRE_RunAppShell [nsEmbedFunctions.cpp : 646 + 0x5]
     r4 = 0xbec91024    r5 = 0x41a022e0    r6 = 0x00000002    r7 = 0x00000003
     r8 = 0x41a23000    r9 = 0x41a28000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec91020    pc = 0x404607dd
    Found by: call frame info
12  libxul.so!mozilla::ipc::MessagePumpForChildProcess::Run [MessagePump.cpp : 198 + 0x3]
     r4 = 0xbec918b8    r5 = 0x41a022e0    r6 = 0x00000002    r7 = 0x00000003
     r8 = 0x41a23000    r9 = 0x41a28000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec91038    pc = 0x40b32db5
    Found by: call frame info
13  libxul.so!MessageLoop::RunInternal [message_loop.cc : 215 + 0x5]
     r4 = 0xbec918b8    r5 = 0x41a1b600    r6 = 0x00000002    r7 = 0x00000003
     r8 = 0x41a23000    r9 = 0x41a28000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec91050    pc = 0x40c3c409
    Found by: call frame info
14  libxul.so!MessageLoop::Run [message_loop.cc : 208 + 0x5]
     r4 = 0xbec918b8    r5 = 0x41a1b600    r6 = 0x00000002    r7 = 0x00000003
     r8 = 0x41a23000    r9 = 0x41a28000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec91058    pc = 0x40c3c4bf
    Found by: call frame info
15  libxul.so!XRE_InitChildProcess [nsEmbedFunctions.cpp : 485 + 0xb]
     r4 = 0xbec918b8    r5 = 0x41a1b600    r6 = 0x00000002    r7 = 0x00000003
     r8 = 0x41a23000    r9 = 0x41a28000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec91070    pc = 0x40460b81
    Found by: call frame info
16  plugin-container!main [MozillaRuntimeMain.cpp : 48 + 0x5]
     r4 = 0xbec91a14    r5 = 0x00000005    r6 = 0x00000006    r7 = 0xbec91a30
     r8 = 0x00000000    r9 = 0x00000000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec919e8    pc = 0x00008451
    Found by: call frame info
17  libc.so!__libc_init [libc_init_dynamic.c : 114 + 0x7]
     r4 = 0x00008414    r5 = 0xbec91a14    r6 = 0x00000006    r7 = 0xbec91a30
     r8 = 0x00000000    r9 = 0x00000000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec919f8    pc = 0x40095a77
    Found by: call frame info
18  0xb00045a9
     r4 = 0x00000000    r5 = 0x00000000    r6 = 0x00000000    r7 = 0x00000000
     r8 = 0x00000000    r9 = 0x00000000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbec91a10    pc = 0xb00045ab
    Found by: call frame info

Comment 1

[Michael Vines [:m1] [:evilmachines]]

Maybe looks a little like bug 823784

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

Let's wait a day or two to see if the fix for bug 823784, which just landed, has a positive impact on this bug.

Comment 3

[Michael Vines [:m1] [:evilmachines]]

Taking a chance and marking this as a dup just to reduce clutter in the system.   If stability test detects this crash again I'll reopen.

Comment 4

[Michael Vines [:m1] [:evilmachines]]

Seen at the tip multiple times, re-opening.  

Test Steps:
1. Receive MT calls continuously.
2. After few hours of run, mini dumps are generated in the phone.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

cc'ing some people from the media team as this seems to be existing non-b2g-specific media code.

Sounds like this for some reason is a commonly hit crasher in B2G. Any ideas for why?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We're crashing deref'ing a null mNodeInfo for the media element, under

void nsHTMLMediaElement::AddRemoveSelfReference()
{
...
  nsIDocument* ownerDoc = OwnerDoc();

I don't really see how this could happen.  The MediaDecoder must be valid because we deref several of its members in MediaDecoder::PlaybackEnded().  MediaDecoder doesn't hold a strong ref to its nsHTMLMediaElement, but we've passed a null check in PlaybackEnded() so Shutdown() must not have been called.

So it looks like either
 - the nsHTMLMediaElement is somehow losing its mNodeInfo
 - the nsHTMLMediaElement is goes away but doesn't call MediaDecoder::Shutdown()
 - ???

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

A null mNodeInfo usually means that you're dereferencing a deleted element. I.e. that AddRemoveSelfReference is being called on an element through a dangling or corrupted pointer.

Comment 8

[Dietrich Ayala (:dietrich)]

Roc, please re-assign to someone in media who can help, thanks!

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: possible fix?

I don't know what the bug is, but I wonder if UpdateReadyStateForData could be doing something that releases the element. This patch makes us check mOwner again just before calling mOwner->PlaybackEnded().

Is there a way to run those tests with this patch?

Comment 10

[Michael Vines [:m1] [:evilmachines]]

Can you just land the patch?

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What do you think roc?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> What do you think roc?

OK.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d44845533eca
Hope this works :-)

Comment 14

[Michael Vines [:m1] [:evilmachines]]

oh, can you uplift this to the b2g branch?  we still have time to make this into the nightly build today and we're planning another round of stability test on tonight's build

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I'll uplift this once it's green on inbound.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5ee31662227f

(Note that this didn't directly apply to b2g18, but I'm pretty sure what I did was right)

Comment 17

[Johnny Stenback (:jst)]

Marking FIXED as this is already on inbound and b2g18 (per b2g endgame rule).

Comment 18

[Ed Morley [:emorley]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is this something we're going to want to uplift to Aurora/Beta?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 698209 [details] [diff] [review]
possible fix?

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Not much
Testing completed (on m-c, etc.): None
Risk to taking this patch (and alternatives if risky): Very low-risk patch.
String or UUID changes made by this patch: None

This patch might prevent some crashes, but the main reason to take it is to make our branches consistent with B2G for testing purposes. It's not a big win but the patch is super low-risk.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/901bcd1719d3
https://hg.mozilla.org/releases/mozilla-beta/rev/7210c963cb89