Crash in mozilla::a11y::DocAccessible::DoInitialUpdate

RESOLVED FIXED in Firefox 57

Status

()

P1
critical
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: davidb, Assigned: aklotz)

Tracking

({crash})

unspecified
mozilla58
Unspecified
Windows 7
crash
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57+ fixed, firefox58 fixed)

Details

(Whiteboard: aes+, crash signature)

Attachments

(3 attachments)

Spun off bug 1383501

This bug was filed from the Socorro interface and is 
report bp-d66274d1-87e3-4728-9de3-a57aa0170913.
=============================================================

We need a way for caller(s) to react smartly when DoInitialUpdate fails. Likely need to be able to return an error code.

Alex this is pretty urgent. Please find AKlotz for high bandwidth details.
I know nothing about this code, and not sure what is a scope of this bug. Aaron probably has more data on it per hg history. Unassigning myself until we have something actionable here.
Assignee: surkov.alexander → nobody
Flags: needinfo?(dbolter)
Flags: needinfo?(aklotz)
Please talk to Aaron for context (we're in meeting now).
Flags: needinfo?(dbolter) → needinfo?(surkov.alexander)
status-firefox57: --- → affected
Priority: -- → P1
I'm still not sure if there's anything we could/should do. We definitely could make DoInitialUpdate to return an error code or throw an exception of whatever, but not sure how it can be used.

Aaron, if you've got ideas on it, please put them here. Otherwise should we wontfix this bug?
Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 4

2 years ago
I think at the very least we need to leave it open while I investigate the failure that triggers the assertion.
Flags: needinfo?(aklotz)
(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #4)
> I think at the very least we need to leave it open while I investigate the
> failure that triggers the assertion.

right, I thought the bug was about DoInitialUpdate change only.
(Assignee)

Updated

2 years ago
Assignee: nobody → aklotz
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 6

2 years ago
Attachment #8909970 - Flags: review?(jmathies)

Updated

2 years ago
Attachment #8909970 - Flags: review?(jmathies) → review+
There are 25 crashes from 3 installations in nightly 57-58 starting with buildid 20170921100141 with signature 'mozilla::mscom::Interceptor::GetInitialInterceptorForIID'.
The moz crash reason is always MOZ_RELEASE_ASSERT((((HRESULT)(hr)) >= 0)).
The crashy line is 'ENSURE_HR_SUCCEEDED(hr);'.
Crash Signature: [@ mozilla::a11y::DocAccessible::DoInitialUpdate] → [@ mozilla::a11y::DocAccessible::DoInitialUpdate] [@ mozilla::mscom::Interceptor::GetInitialInterceptorForIID]
[Tracking Requested - why for this release]:
There are 1622 crashes for 57 installations in 57.0b0 (dev edition) with signature 'mozilla::a11y::DocAccessible::DoInitialUpdate'
tracking-firefox57: --- → ?
There's a mixture of a11y clients in here, about 50%/50% oop/in-proc.
Tracked for 57 based on comment 11
tracking-firefox57: ? → +
(Assignee)

Comment 15

2 years ago
This patch adds Environment to ProxyStream. When ProxyStream sees an Environment object, immediately before (un)marshaling, it pushes the environment, and then pops it after (un)marshaling. This allows us to do any setup and teardown that is required, *on the thread where the (un)marshaling is to be performed*.

In the case of IAccessible, we use it to push and pop the required activation context.
Attachment #8914107 - Flags: review?(jmathies)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Comment on attachment 8914107 [details] [diff] [review]
Add the concept of an "Environment" to ProxyStreams

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

Looks great, lets hope for good results!
Attachment #8914107 - Flags: review?(jmathies) → review+
btw what's the overhead of the activation calls here? I didn't get a sense for what that might be.
(Assignee)

Comment 18

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f76f2dcae8f17bb43ff125b82b36c70a6cc2334d
Bug 1399557: Add Environment to mscom::ProxyStream and define it for IAccessible; r=jimm
(Assignee)

Comment 19

2 years ago
(In reply to Jim Mathies [:jimm] from comment #17)
> btw what's the overhead of the activation calls here? I didn't get a sense
> for what that might be.

Should be pretty light -- activation context pushes and pops are buried all over the place throughout internal Win32 code (eg WndProcs are wrapped by them), so I'd think that they'd be pretty low overhead.
(Assignee)

Comment 21

2 years ago
Comment on attachment 8914107 [details] [diff] [review]
Add the concept of an "Environment" to ProxyStreams

Approval Request Comment
[Feature/Bug causing the regression]: a11y+e10s
[User impact if declined]: Potential for continued crashing for a11y users (note that this crash shows up as bug 1383501 on beta)
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Not a complicated change
[String changes made/needed]: None
Attachment #8914107 - Flags: approval-mozilla-beta?
(Assignee)

Comment 22

2 years ago
Gonna resolve as fixed for now.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Updated

2 years ago
status-firefox58: affected → fixed
status-firefox56: --- → unaffected
status-firefox-esr52: --- → unaffected
Comment on attachment 8914107 [details] [diff] [review]
Add the concept of an "Environment" to ProxyStreams

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

Fix a *few* crashes, taking it.
Attachment #8914107 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 26

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/76a43510d785
status-firefox57: affected → fixed
Depends on: 1406054
(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #21)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Aaron's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Crashes with these signatures are still showing up.  For example it is
#8 topcrash in the Windows nightly 20171005220204, and similarly in
20171005100211.
Still getting `mozilla::mscom::Interceptor::GetInitialInterceptorForIID` crashes in latest Nightly. Is this expected?
Flags: needinfo?(aklotz)
This is topcrash #6 in the Windows nightly 20171012105833.
Flags: needinfo?(dbolter)
(In reply to (pto > 10/23) Jim Chen [:jchen] [:darchons] from comment #29)
> Still getting `mozilla::mscom::Interceptor::GetInitialInterceptorForIID`
> crashes in latest Nightly. Is this expected?

Yes I think we still expect some causes to remain... Aaron can give details.
Flags: needinfo?(dbolter)
(Assignee)

Comment 32

a year ago
(In reply to Jim Chen [:jchen] [:darchons] from comment #29)
> Still getting `mozilla::mscom::Interceptor::GetInitialInterceptorForIID`
> crashes in latest Nightly. Is this expected?

Yes. We intentionally left diagnostic asserts in place on Nightly so that we could continue to investigate the root cause of these problems. On Beta these asserts go away and we are able to recover from the error to the extent that we won't crash (though a11y APIs might not work right).
Flags: needinfo?(aklotz)

Comment 33

a year ago
the user comment at bp-5733d27f-7212-4b42-8bf6-4c0950171208 would for example say "搜狗输入法+狐火=崩溃" (machine translation: "Sogou input method + fox = collapse")
and a good share of those crashes is coming from chinese locales.
(In reply to Aaron Klotz [:aklotz] from comment #32)
> Yes. We intentionally left diagnostic asserts in place on Nightly so that we
> could continue to investigate the root cause of these problems. [..]

Just to note, this is #12 topcrash in the Windows nightly 20171207100053, and
judging from the graph above, seems to have been crashing fairly frequently for
at least the last month.
Flags: needinfo?(aklotz)

Updated

a year ago
Crash Signature: [@ mozilla::a11y::DocAccessible::DoInitialUpdate] [@ mozilla::mscom::Interceptor::GetInitialInterceptorForIID] → [@ mozilla::a11y::DocAccessible::DoInitialUpdate] [@ mozilla::mscom::Interceptor::GetInitialInterceptorForIID] [@ mozilla::a11y::CreateHolderFromAccessible]
See Also: → bug 1434944
There are 5 crashes with signature "mozilla::a11y::CreateHolderFromAccessible" in nightly 60 starting with buildid 20180214111549 (see [1]). The moz_crash_reason is always MOZ_RELEASE_ASSERT((((HRESULT)(hr)) >= 0)).
:aklotz, Should I reopen a new bug ?

[1] http://bit.ly/2oaqcKx
(Assignee)

Comment 36

a year ago
No, we've since landed some patches to get rid of those crashes on Nightly.
Flags: needinfo?(aklotz)
See Also: → bug 1455745
You need to log in before you can comment on or make changes to this bug.