1399557 - Crash in mozilla::a11y::DocAccessible::DoInitialUpdate

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

[alexander :surkov (:asurkov)]

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.

Comment 2

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

Please talk to Aaron for context (we're in meeting now).

Comment 3

[alexander :surkov (:asurkov)]

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?

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

I think at the very least we need to leave it open while I investigate the failure that triggers the assertion.

Comment 5

[alexander :surkov (:asurkov)]

(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.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add diagnostic asserts

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/282b2976adfdb47f3756af5c4ce11c6d9d0ae798
Bug 1399557: Add diagnostic asserts to interceptor creation code; r=jimm

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/789e21a08030b00acd832a58268680dede2e7a1b
Bug 1399557: Follow-up: Add check for expected error code; r=bustage

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/282b2976adfd
https://hg.mozilla.org/mozilla-central/rev/789e21a08030

Comment 10

[Calixte Denizet (:calixte)]

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);'.

Comment 11

[Calixte Denizet (:calixte)]

[Tracking Requested - why for this release]:
There are 1622 crashes for 57 installations in 57.0b0 (dev edition) with signature 'mozilla::a11y::DocAccessible::DoInitialUpdate'

Comment 12

[Jim Mathies [:jimm]]

Showing up as #8 on 58. The installations numbers are really low though.

https://crash-stats.mozilla.com/signature/?date=%3C2017-09-24T19%3A59%3A43%2B00%3A00&date=%3E%3D2017-09-17T19%3A59%3A43%2B00%3A00&product=Firefox&version=58.0a1&signature=mozilla%3A%3Amscom%3A%3AInterceptor%3A%3AGetInitialInterceptorForIID

Comment 13

[Jim Mathies [:jimm]]

There's a mixture of a11y clients in here, about 50%/50% oop/in-proc.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 57 based on comment 11

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add the concept of an "Environment" to ProxyStreams

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.

Comment 16

[Jim Mathies [:jimm]]

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!

Comment 17

[Jim Mathies [:jimm]]

btw what's the overhead of the activation calls here? I didn't get a sense for what that might be.

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

(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 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

Gonna resolve as fixed for now.

Comment 23

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b4ff508e259db6a3abe5cb8ded85c219321d64a

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add the concept of an "Environment" to ProxyStreams (rebased for 57 beta)

Comment 25

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/76a43510d785

Comment 27

[Andrei Vaida [:avaida]]

(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.

Comment 28

[Julian Seward [:jseward]]

Crashes with these signatures are still showing up.  For example it is
#8 topcrash in the Windows nightly 20171005220204, and similarly in
20171005100211.

Comment 29

[(inactive) Jim Chen [:jchen] [:darchons]]

Still getting `mozilla::mscom::Interceptor::GetInitialInterceptorForIID` crashes in latest Nightly. Is this expected?

Comment 30

[Julian Seward [:jseward]]

This is topcrash #6 in the Windows nightly 20171012105833.

Comment 31

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

(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.

Comment 32

[(No longer employed by Mozilla) Aaron Klotz]

(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).

Comment 33

[[:philipp]]

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.

Comment 34

[Julian Seward [:jseward]]

(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.

Comment 35

[Calixte Denizet (:calixte)]

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

Comment 36

[(No longer employed by Mozilla) Aaron Klotz]

No, we've since landed some patches to get rid of those crashes on Nightly.