Closed Bug 1399557 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Disability Access APIs, defect, P1)

Unspecified
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: davidb, Assigned: bugzilla)

References

Details

(Keywords: crash, Whiteboard: aes+)

Crash Data

Attachments

(3 files)

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)
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)
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: nobody → aklotz
Attachment #8909970 - Flags: review?(jmathies)
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'
There's a mixture of a11y clients in here, about 50%/50% oop/in-proc.
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)
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.
(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.
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?
Gonna resolve as fixed for now.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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+
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)
(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)
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)
Crash Signature: [@ mozilla::a11y::DocAccessible::DoInitialUpdate] [@ mozilla::mscom::Interceptor::GetInitialInterceptorForIID] → [@ mozilla::a11y::DocAccessible::DoInitialUpdate] [@ mozilla::mscom::Interceptor::GetInitialInterceptorForIID] [@ mozilla::a11y::CreateHolderFromAccessible]
See Also: → 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
No, we've since landed some patches to get rid of those crashes on Nightly.
Flags: needinfo?(aklotz)
See Also: → 1455745
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: