1228400 - crash in mozilla::dom::PBrowserChild::SendPDocAccessibleConstructor

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

This bug was filed from the Socorro interface and is 
report bp-414ed0b4-d069-41b0-885a-c4ada2151124.
=============================================================

More here: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Adom%3A%3APBrowserChild%3A%3ASendPDocAccessibleConstructor

Comment 1

[Bob Owen (:bobowen)]

All the reports I spot checked are crashing on a line which dereferences the actor (first argument to SendPDocAccessibleConstructor).

That doesn't make any sense as it's been null checked just above.
The line calls Register, which dereferences the mManager (which will be the PContentChild).
So, could this be getting called during some sort of tear down where the PContentChild has already gone away?

Has anyone been able to find steps to reproduce this?

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to Bob Owen (:bobowen) from comment #1)
> All the reports I spot checked are crashing on a line which dereferences the
> actor (first argument to SendPDocAccessibleConstructor).
> 
> That doesn't make any sense as it's been null checked just above.
> The line calls Register, which dereferences the mManager (which will be the
> PContentChild).

can you post disassembly  for one of the crashes? I'm curious which of the actor or mManager is actually null.

It certainly doesn't make sense its the actor we are creating since we just created that with new and yeah its also null checked.

> So, could this be getting called during some sort of tear down where the
> PContentChild has already gone away?

I would guess not looking at the first stack I saw it was while handling a normal async message from the parent about vsync nothing shutdown related.

> Has anyone been able to find steps to reproduce this?

I haven't really tried to reproduce, so I suspect not.

Comment 3

[Bob Owen (:bobowen)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> (In reply to Bob Owen (:bobowen) from comment #1)
> > All the reports I spot checked are crashing on a line which dereferences the
> > actor (first argument to SendPDocAccessibleConstructor).
> > 
> > That doesn't make any sense as it's been null checked just above.
> > The line calls Register, which dereferences the mManager (which will be the
> > PContentChild).
> 
> can you post disassembly  for one of the crashes? I'm curious which of the
> actor or mManager is actually null.

Here's the c++ source and disassembly from the start of SendPDocAccessibleConstructor:

auto PBrowserChild::SendPDocAccessibleConstructor(
        PDocAccessibleChild* actor,
        PDocAccessibleChild* aParentDoc,
        const uint64_t& aParentAcc) -> PDocAccessibleChild*
{
00007FFD6D6D8D30  push        rbx  
00007FFD6D6D8D32  push        rbp  
00007FFD6D6D8D33  push        rsi  
00007FFD6D6D8D34  push        rdi  
00007FFD6D6D8D35  push        r14  
00007FFD6D6D8D37  sub         rsp,50h  
00007FFD6D6D8D3B  mov         rax,qword ptr [__security_cookie (07FFD6F6221D0h)]  
00007FFD6D6D8D42  xor         rax,rsp  
00007FFD6D6D8D45  mov         qword ptr [rsp+40h],rax  
00007FFD6D6D8D4A  mov         r14,r9  
00007FFD6D6D8D4D  mov         rbp,r8  
00007FFD6D6D8D50  mov         rdi,rdx  
00007FFD6D6D8D53  mov         rsi,rcx  
    if ((!(actor))) {
00007FFD6D6D8D56  test        rdx,rdx  
00007FFD6D6D8D59  jne         mozilla::dom::PBrowserChild::SendPDocAccessibleConstructor+32h (07FFD6D6D8D62h)  
        NS_WARNING("Error constructing actor PDocAccessibleChild");
        return nullptr;
00007FFD6D6D8D5B  xor         eax,eax  
00007FFD6D6D8D5D  jmp         mozilla::dom::PBrowserChild::SendPDocAccessibleConstructor+166h (07FFD6D6D8E96h)  
    }
    (actor)->mId = Register(actor);
00007FFD6D6D8D62  lea         rbx,[rcx+10h]  
00007FFD6D6D8D66  mov         rcx,rbx  
00007FFD6D6D8D69  mov         rax,qword ptr [rbx]  
00007FFD6D6D8D6C  call        qword ptr [rax]  

End of disassembly.

It's failing on the second to last instruction, because rbx contains 0x68 and so is an invalid location.
So it seems that "this" is corrupt (0x58) presumably from the static cast in NotificationController::WillRefresh.
That do_GetInterface into tabChild must be returning nullptr, which when static cast to TabChild* gives 0x58.
So, we need a null check on tabChild.

Comment 4

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #3)

> That do_GetInterface into tabChild must be returning nullptr, which when
> static cast to TabChild* gives 0x58.

Erm, I meant when cast to TabChild* and implicitly converted to a PBrowserChild* for the call to SendPDocAccessibleConstructor gives 0x58.

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: null check tabChild before notifying the parent process about new child documents

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8698034 [details] [diff] [review]
null check tabChild before notifying the parent process about new child documents

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

Thanks both!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5356e384c04

Comment 8

[Carsten Book [:Tomcat]]

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