Closed Bug 1246447 Opened 4 years ago Closed 3 years ago

crash in mozilla::a11y::DocAccessible::ARIAAttributeChanged

Categories

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

Unspecified
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: davidb, Assigned: surkov)

References

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

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?
Attached file 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?
Flags: needinfo?(dbolter)
Note you have to make sure accessibility is activated. Doesn't repro on Mac, I think Marco is checking Windows.
Flags: needinfo?(dbolter)
Duplicate of this bug: 1249247
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.
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
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!
(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?
Assignee: nobody → surkov.alexander
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
Attached patch patchSplinter Review
wasn't able to reproduce, a null check+assertion then
Attachment #8777429 - Flags: review?(yzenevich)
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?
Attachment #8777429 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/66256409133057d37962b8aaeed02532f8883f5d
Bug 1246447 - crash in mozilla::a11y::DocAccessible::ARIAAttributeChanged, r=yzen
https://hg.mozilla.org/mozilla-central/rev/662564091330
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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).
Flags: needinfo?(surkov.alexander)
Here's another more "official" (non-try) failure:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=43638
is it intermittent? is there a way I can reproduce it on a local machine?
Flags: needinfo?(surkov.alexander)
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.
(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?
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.
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 :-(
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.)
(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.
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.)
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.
I do not see that assertion on linux64 debug build in that test. And there also isn't the "TypeError: tab is undefined" message.
no luck to reproduce it on windows

would it make sense to open a new bug for the issue?
Depends on: 1294500
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.
Alexander, do you want to uplift that to aurora & beta? Thanks
Flags: needinfo?(surkov.alexander)
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.
Attached patch aurora patchSplinter Review
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
Flags: needinfo?(surkov.alexander)
Attachment #8783964 - Flags: approval-mozilla-aurora?
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)?
Flags: needinfo?(surkov.alexander)
Comment on attachment 8783964 [details] [diff] [review]
aurora patch

Crash fix, not null check, Aurora50+
Attachment #8783964 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
as Jorg said, no extra patch is needed
Flags: needinfo?(surkov.alexander)
Keywords: checkin-needed
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.
Keywords: checkin-needed
(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.
Too late for 49 beta, but good that this will be fixed in 50.
You need to log in before you can comment on or make changes to this bug.