1246447 - crash in mozilla::a11y::DocAccessible::ARIAAttributeChanged

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-89f73c2c-50ee-48c7-b842-9c5322160126.
=============================================================

More reports here: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Aa11y%3A%3ADocAccessible%3A%3AARIAAttributeChanged

Apparently we missed a null check case. Do we know how we have a null parent?

Comment 1

[Loic]

Attachment: index.html

Bug 1249247 has been filed with the same crash signature + a testcase.

David, are you able to reproduce the crash with the testcase I attached?

Comment 2

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

Note you have to make sure accessibility is activated. Doesn't repro on Mac, I think Marco is checking Windows.

Comment 4

[Marco Zehe (:MarcoZ)]

I cannot reproduce on Windows with NVDA. Note that I am using NVDA's virtual buffer and don't use the mouse. I canot get it to crash with the test case. I can change dates, months, I can even clear and close it without problems. Opening it itself happens automatically as soon as NVDA puts focus on the widget in its virtual buffer and causes Firefox to focus it. So... No luck on this end with current 47.0a1 2016-02-17 nightly build.

Comment 5

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

Lot's of comments in crash-stats mention a date picker seeming to confirm the STR from 1249247 and the testcase posted here. For example someone claims it always happens when picking dates on this site: https://www.afvclub.com/search-resorts

Comment 6

[Manuel Strehl]

I'm the reporter of bug 1249247. I can confirm, that my FF crashes also:

* on that AFV Club page, when trying to pick a date, and
* with a clean profile, ruling out any plugins.

If I can provide any more metrics, just ask!

Comment 7

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

(In reply to Manuel Strehl from comment #6)
 > If I can provide any more metrics, just ask!

Thanks!

Alex I think this is a regression from the aria-hidden work in bug 786143. Can you take this?

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'mozilla::a11y::DocAccessible::ARIAAttributeChanged':
 - nightly (version 50): 0 crashes from 2016-06-06.
 - aurora  (version 49): 5 crashes from 2016-06-07.
 - beta    (version 48): 14 crashes from 2016-06-06.
 - release (version 47): 1540 crashes from 2016-05-31.
 - esr     (version 45): 535 crashes from 2016-04-07.

Crash volume on the last weeks:
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       0       0       0       0       0       0       0
 - aurora        0       0       0       4       1       0       0
 - beta          7       0       2       2       0       0       3
 - release     214     192     221     224     191     216     179
 - esr          37      52      49      27      47      53      55

Affected platforms: Windows, Mac OS X, Linux

Comment 9

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c133a35cb19

Comment 10

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c4dd5c923ca

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch

wasn't able to reproduce, a null check+assertion then

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8777429 [details] [diff] [review]
patch

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

code looks good

::: accessible/generic/DocAccessible.cpp
@@ +764,5 @@
>      accessible = this;
>    }
>  
> +  if (!accessible->IsBoundToParent()) {
> +    MOZ_ASSERT_UNREACHABLE("DOM attribute change on not in tree accessible.");

nit: not in tree accessible -> accessible detached from tree?

Comment 13

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66256409133057d37962b8aaeed02532f8883f5d
Bug 1246447 - crash in mozilla::a11y::DocAccessible::ARIAAttributeChanged, r=yzen

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Jorg K (CEST = GMT+2)]

With this change a debug build of Thunderbird now crashes in our Mozmill test suite, for example here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cb176244084fc11fb237c15a14cf9eacddf92e1c
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cb176244084fc11fb237c15a14cf9eacddf92e1c&selectedJob=20696

Is that expected or intentional?
Looks like our test that's failing is:
08:51:19     INFO -  JavaScript error: chrome://messenger/content/tabmail.xml, line 1073: TypeError: tab is undefined
08:51:21     INFO -  ++DOMWINDOW == 157 (0x7fb10d6e9800) [pid = 5096] [serial = 238] [outer = 0x7fb10c8d8800]
08:51:21     INFO -  Assertion failure: false (MOZ_ASSERT_UNREACHABLE: DOM attribute change on accessible detached from tree), at /builds/slave/tb-try-c-cen-l64-d-00000000000/build/mozilla/accessible/generic/DocAccessible.cpp:768

Maybe it has to do with the JS error that comes before (although I doubt that).

Comment 16

[Jorg K (CEST = GMT+2)]

Here's another more "official" (non-try) failure:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=43638

Comment 17

[alexander :surkov (:asurkov)]

is it intermittent? is there a way I can reproduce it on a local machine?

Comment 18

[Jorg K (CEST = GMT+2)]

No, it's permanent on a debug build. Can you build Thunderbird?
In comment #15 I forgot to paste the test that's failing, it's:
TEST-START | /builds/slave/test/build/tests/mozmill/message-header/test-header-toolbar.js

So with a local debug build of TB you can just run the test locally and see whether it fails. Or I could do that for you in the next 24-48 hours.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #18)
> No, it's permanent on a debug build. Can you build Thunderbird?

I think so.

> In comment #15 I forgot to paste the test that's failing, it's:
> TEST-START |
> /builds/slave/test/build/tests/mozmill/message-header/test-header-toolbar.js
> 
> So with a local debug build of TB you can just run the test locally and see
> whether it fails.

is there a command to run this specific test?

Comment 20

[Jorg K (CEST = GMT+2)]

Sure. You
cd obj*
mozmake SOLO_TEST=message-header/test-header-toolbar.js mozmill-one
(on Windows, perhaps just 'make' on Linux).
It's quite a show having TB being driven by the hand of a ghost.

I've applied the patch here to a local build and it does *NOT* crash on Windows.

That can have one of two reasons:
1) My local build needs to be refreshed (it's a few days old)
2) The crash is not on Windows in general. The links I posted in comment #15 and comment #16
   were for Linux 64bit (not showing on Mac or Linux 32 bit).
   We had Windows bustage until today, so unless someone lands something
   on the C-C tree and that triggers a rerun and rebuild on Windows we don't know.

So how about:
1) I refresh my local build later today
2) We wait for Treeherder results for Windows to become available.
   Actually, I've triggered Mozmill tests for Win XP/7 debug, so we'll know soon:
   https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=7b9fbef870b2d735e06105bfe4e9dc09518545dd
   Watch the grey "Z"s. Oh, damn, the test executed without compiling the binary first and failed.
   So I triggered a build (grey "B") and I'll add the Mozmill tests once the build is done in two hours.
   I'll keep you posted.

Comment 21

[Jorg K (CEST = GMT+2)]

OK, Windows has built and we see the error in both debug runs:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=7b9fbef870b2d735e06105bfe4e9dc09518545dd
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=7b9fbef870b2d735e06105bfe4e9dc09518545dd&selectedJob=43663
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=7b9fbef870b2d735e06105bfe4e9dc09518545dd&selectedJob=43662

I've also refreshed my local build and this
mozmake SOLO_TEST=message-header/test-header-toolbar.js mozmill-one
does still *not* crash. That doesn't make it easy to track down :-(

Comment 22

[Jorg K (CEST = GMT+2)]

Now that we have reopened the C-C tree since recent bustage has been fixed, we see this failure on Linux and Windows in the Mozmill debug runs, the red "Z"s:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=adc56f0cd5b514b27a96cebb31afe02503971b0c
(Mac seems to crash for some other reason.)

Comment 23

[alexander :surkov (:asurkov)]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #22)
> Now that we have reopened the C-C tree since recent bustage has been fixed,
> we see this failure on Linux and Windows in the Mozmill debug runs, the red
> "Z"s:
> https://treeherder.mozilla.org/#/jobs?repo=comm-
> central&revision=adc56f0cd5b514b27a96cebb31afe02503971b0c
> (Mac seems to crash for some other reason.)

right, I get on OS X
"Assertion failure: aData[middle] == 0xFF, at /Users/heh/mozilla/comm/mozilla/gfx/2d/DrawTargetSkia.cpp:192"
TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed

I'm curious if this one is fixed, then would it reveal the problem? I could try to build on VM Windows though.

Comment 24

[Jorg K (CEST = GMT+2)]

Thanks for looking into it!

(In reply to alexander :surkov from comment #23)
> > (Mac seems to crash for some other reason.)
> right, I get on OS X
> "Assertion failure: aData[middle] == 0xFF, at
> /Users/heh/mozilla/comm/mozilla/gfx/2d/DrawTargetSkia.cpp:192"
Sorry for not being more precise before. I since have tracked down this other problem:
Bug 1279063 comment #18.
(Now you know how it feels to be a Thunderbird developer constantly chasing after bustage caused by M-C changes.)

Comment 25

[Jorg K (CEST = GMT+2)]

I forgot to say: Before you set up a VM, I suggest to remove the asserts from here
https://hg.mozilla.org/mozilla-central/rev/f1f81ccfb6d4#l1.69
for testing.

Comment 26

[:aceman]

I do not see that assertion on linux64 debug build in that test. And there also isn't the "TypeError: tab is undefined" message.

Comment 27

[alexander :surkov (:asurkov)]

no luck to reproduce it on windows

would it make sense to open a new bug for the issue?

Comment 28

[Jorg K (CEST = GMT+2)]

Thanks for looking into it. I couldn't reproduce it either running the test locally.

(In reply to alexander :surkov from comment #27)
> would it make sense to open a new bug for the issue?
Done, bug 1294500.

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Alexander, do you want to uplift that to aurora & beta? Thanks

Comment 30

[Marco Zehe (:MarcoZ)]

Note that if you uplift this one, bug 1294500 might need uplifting, too, to fix Thunderbird builds that depend on the given m-c version.

Comment 31

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68d9fcd344c0

Comment 32

[alexander :surkov (:asurkov)]

Attachment: aurora patch

Approval Request Comment
[Feature/regressing bug #]:bug 786143
[User impact if declined]:crashes
[Describe test coverage new/current, TreeHerder]:running on trunk, the code has fair coverage by mochitest
[Risks and why]: low, null check
[String/UUID change made/needed]:no

Comment 33

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

Hi Alexander, do we need to also uplift the fix from bug 1294500 to Aurora50 so we don't regress TH builds (See comment 30)?

Comment 34

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

Comment on attachment 8783964 [details] [diff] [review]
aurora patch

Crash fix, not null check, Aurora50+

Comment 35

[Jorg K (CEST = GMT+2)]

(In reply to Ritu Kothari (:ritu) from comment #33)
> Hi Alexander, do we need to also uplift the fix from bug 1294500 to Aurora50
> so we don't regress TB builds (See comment 30)?
The patch here combines the original patch from this bug with the one from bug 1294500. So uplifting the special Aurora patch is sufficient. Thanks for keeping TB in mind.

Comment 36

[alexander :surkov (:asurkov)]

as Jorg said, no extra patch is needed

Comment 37

[Jorg K (CEST = GMT+2)]

Taking off "checkin-needed", as far as I know that's for M-C only, I think they control Aurora some other way. Correct me if I'm wrong.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3dd6a13350a6

Comment 39

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

(In reply to Jorg K (GMT+2, PTO during summer) from comment #35)
> (In reply to Ritu Kothari (:ritu) from comment #33)
> > Hi Alexander, do we need to also uplift the fix from bug 1294500 to Aurora50
> > so we don't regress TB builds (See comment 30)?
> The patch here combines the original patch from this bug with the one from
> bug 1294500. So uplifting the special Aurora patch is sufficient. Thanks for
> keeping TB in mind.

Got it. Thanks Jorg and Alex.

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 49 beta, but good that this will be fixed in 50.