Crash in mozilla::ipc::LogicError | mozilla::a11y::PDocAccessibleChild::SendScrollingEvent

VERIFIED FIXED in Firefox 63

Status

()

defect
--
critical
VERIFIED FIXED
Last year
Last year

People

(Reporter: calixte, Assigned: eeejay)

Tracking

(Blocks 1 bug, {crash, regression})

Trunk
mozilla63
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62 unaffected, firefox63 verified)

Details

(crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is
report bp-e089ebda-954e-4e54-8289-1cdaf0180816.
=============================================================

Top 10 frames of crashing thread:

0 mozglue.dll MOZ_CrashOOL mfbt/Assertions.cpp:33
1 xul.dll mozilla::ipc::LogicError ipc/glue/ProtocolUtils.cpp:309
2 xul.dll mozilla::a11y::PDocAccessibleChild::SendScrollingEvent ipc/ipdl/PDocAccessibleChild.cpp:494
3 xul.dll mozilla::a11y::Accessible::HandleAccEvent accessible/generic/Accessible.cpp:958
4 xul.dll mozilla::a11y::AccessibleWrap::HandleAccEvent accessible/windows/msaa/AccessibleWrap.cpp:1233
5 xul.dll nsEventShell::FireEvent accessible/base/nsEventShell.cpp:46
6 xul.dll void mozilla::a11y::DocAccessible::DispatchScrollingEvent accessible/generic/DocAccessible.cpp:2465
7 xul.dll mozilla::a11y::DocAccessible::ScrollPositionDidChange accessible/generic/DocAccessible.cpp:633
8 xul.dll void mozilla::ScrollFrameHelper::ScrollToImpl layout/generic/nsGfxScrollFrame.cpp:3014
9 xul.dll void mozilla::ScrollFrameHelper::CompleteAsyncScroll layout/generic/nsGfxScrollFrame.cpp:2234

=============================================================

There is 1 crash in nightly 63 with buildid 20180816100035. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1479591.

[1] https://hg.mozilla.org/mozilla-central/rev?node=da7e0c710c7d
Flags: needinfo?(eitan)
This is my hypothesis, let's see if Jamie agrees.

1. This patch introduces a new event that is dispatched immediately when a scroll starts.
2. This crash is happening on startup when the event is dispatched but PDocAccessibleChild is not yet constructed.
3. I think this has to do with DocAccessibleChild::ConstructChildDocInParentProcess and how it sends a deferred constructor to the parent.

Maybe a way to fix this is to check that ipcDoc has a manager before sending events?
Flags: needinfo?(eitan) → needinfo?(jteh)
(In reply to Eitan Isaacson [:eeejay] from comment #1)
> 1. This patch introduces a new event that is dispatched immediately when a
> scroll starts.

Why would a scroll start so early, though? I guess anything is possible...

> 2. This crash is happening on startup when the event is dispatched but
> PDocAccessibleChild is not yet constructed.
> 3. I think this has to do with
> DocAccessibleChild::ConstructChildDocInParentProcess and how it sends a
> deferred constructor to the parent.

That sounds reasonable.

DocAccessibleChild::Send*Event need to be overridden for Windows to push deferred events if the doc isn't yet constructed in the parent process. For example, we have overrides for SendHideEvent, SendRoleChangedEvent, etc. See bug 1314707 for more details. I notice there isn't such an override for SendScrollingEvent, which is probably why this is barfing.
Flags: needinfo?(jteh)
This crashes reliably for me at the end of loading in Google Docs. Does not happen if you load in a background tab.
(In reply to James Teh [:Jamie] from comment #2)
> (In reply to Eitan Isaacson [:eeejay] from comment #1)
> > 1. This patch introduces a new event that is dispatched immediately when a
> > scroll starts.
> 
> Why would a scroll start so early, though? I guess anything is possible...
> 

The stack suggests that this is a focus-induced scroll. Probably some kind of onload.
Comment on attachment 9002621 [details] [diff] [review]
Defer scrolling events until after ipc doc construction. r?Jamie

Thanks. I really wish we didn't need to repeat all of this boilerplate for deferred events, but such is life.
Attachment #9002621 - Flags: review?(jteh) → review+
Assignee: nobody → eitan
Keywords: checkin-needed
Pushed by dvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34996338b92f
Defer scrolling events until after ipc doc construction. r=Jamie
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/34996338b92f
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
No more crashes since the patch landed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.