onclick change may make a node accessible or make it inaccessible, it changes neither its role or type, thus there's no point to recreate the accessible and whole its subtree. in the web the author often put event handlers on top elements, thus the setting/unsetting the event handlers may be a performance issue, as well it may be a stability issue, I do see this code path in crashes like in bug 1309686.

There are two approaches here: 1) add AccessibleType() method to detect whether which type a node belongs to. If no type then shutdown the accessible, if it has type, then do nothing. 2) if a node has onclick and it's not accessible then create an accessible for it. The downside of 2nd is we may keep extra accessibles in the tree, which serves no propose, but should be harmless overall; the benefit is zero costs tree updates, which makes it quite tempting.

Is there an impact on ATs here? When you say "we may keep extra accessibles in the tree", I assume you mean that removing onClick won't remove the accessible? That's fine from my perspective; the less churn in the tree, the better. What if a node didn't have onClick and then gains it? I assume that will be a tree update as usual?

(In reply to James Teh [:Jamie] from comment #2) > Is there an impact on ATs here? When you say "we may keep extra accessibles > in the tree", I assume you mean that removing onClick won't remove the > accessible? That's fine from my perspective; the less churn in the tree, the > better. What if a node didn't have onClick and then gains it? I assume that > will be a tree update as usual? correct, the proposal (#2 option) is: if onclick goes away, then node stays accessible, if inaccessible node gets onclick, then it becomes accessible I cannot think of any negative impact on ATs, I cc'ed you though to double check it.

let's keep things shorter

Comment on attachment 8847120 [details] [diff] [review] patch Review of attachment 8847120 [details] [diff] [review]: ----------------------------------------------------------------- this is much more clear now. ::: accessible/tests/mochitest/treeupdate/test_textleaf.html @@ +47,5 @@ > } > > function removeOnClickAttr(aID) > { > + var node = getNode(aID); nits: var -> let

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c05090a2cd01ad71df1d3840ca17818d8b6cdba Bug 1342433 - don't recreate a tree on onclick changes, r=yzen

Backed out for failing browser_treeupdate_listener.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/76614caf4ddc829499cf8739031878f29a28e465 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7c05090a2cd01ad71df1d3840ca17818d8b6cdba&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=84059804&repo=mozilla-inbound [task 2017-03-15T17:04:47.699205Z] 17:04:47 INFO - TEST-START | accessible/tests/browser/e10s/browser_treeupdate_listener.js [task 2017-03-15T17:05:32.793902Z] 17:05:32 INFO - TEST-INFO | started process screentopng [task 2017-03-15T17:05:34.187180Z] 17:05:34 INFO - TEST-INFO | screentopng: exit 0 [task 2017-03-15T17:05:34.187601Z] 17:05:34 INFO - Buffered messages logged at 17:04:47 [task 2017-03-15T17:05:34.187799Z] 17:05:34 INFO - Entering test bound [task 2017-03-15T17:05:34.189742Z] 17:05:34 INFO - TEST-PASS | accessible/tests/browser/e10s/browser_treeupdate_listener.js | Accessible document present. - [task 2017-03-15T17:05:34.191241Z] 17:05:34 INFO - TEST-PASS | accessible/tests/browser/e10s/browser_treeupdate_listener.js | Check that parent is not accessible. - [task 2017-03-15T17:05:34.193583Z] 17:05:34 INFO - TEST-PASS | accessible/tests/browser/e10s/browser_treeupdate_listener.js | Check that child is not accessible. - [task 2017-03-15T17:05:34.197552Z] 17:05:34 INFO - TEST-PASS | accessible/tests/browser/e10s/browser_treeupdate_listener.js | Accessible document present. - [task 2017-03-15T17:05:34.199669Z] 17:05:34 INFO - TEST-PASS | accessible/tests/browser/e10s/browser_treeupdate_listener.js | Wrong value of property 'role' for ['span@id="parent" node', address: [object HTMLSpanElement], role: text, address: [xpconnect wrapped nsIAccessible]]. - [task 2017-03-15T17:05:34.202845Z] 17:05:34 INFO - TEST-PASS | accessible/tests/browser/e10s/browser_treeupdate_listener.js | Wrong first child of ['span@id="parent" node', address: [object HTMLSpanElement], role: text, address: [xpconnect wrapped nsIAccessible]] - [task 2017-03-15T17:05:34.205019Z] 17:05:34 INFO - TEST-PASS | accessible/tests/browser/e10s/browser_treeupdate_listener.js | Wrong last child of ['span@id="parent" node', address: [object HTMLSpanElement], role: text, address: [xpconnect wrapped nsIAccessible]] - [task 2017-03-15T17:05:34.209121Z] 17:05:34 INFO - Buffered messages finished [task 2017-03-15T17:05:34.213448Z] 17:05:34 INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/e10s/browser_treeupdate_listener.js | Test timed out -

removing 'remove onclick part', since no tree changes means no events to catch, and possibly it's not that important to test that the tree is not changed in this case.

Comment on attachment 8847730 [details] [diff] [review] patch +browser test fix Review of attachment 8847730 [details] [diff] [review]: ----------------------------------------------------------------- thanks

https://hg.mozilla.org/integration/mozilla-inbound/rev/3873a60605b37e31f0d39bd65f2aa528f14734d5 Bug 1342433 - onclick changes shouldn't recreate the tree, r=yzen

Comment on attachment 8847730 [details] [diff] [review] patch +browser test fix Approval Request Comment [Feature/Bug causing the regression]:unknown [User impact if declined]:required to continue investigation of bug 1321384, if fixes crashes introduced by the diagnostic patch of that bug above [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]:this one only [Is the change risky?]:fair risk [Why is the change risky/not risky?]:it changes how we build the accessible tree, which is unlikely will have any negative effect on screen readers [String changes made/needed]:no

Comment on attachment 8847730 [details] [diff] [review] patch +browser test fix This patch helps investigate the crash and include tests. Aurora54+.

Alexander, can you make a case for landing this on beta as well? (to fix bug 1321384)? We could still land this in 53 beta 10

Comment on attachment 8847730 [details] [diff] [review] patch +browser test fix Approval Request Comment [Feature/Bug causing the regression]:unknown [User impact if declined]:high amount of crashes, bug 1321384 [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]:shouldn't have any deps [Is the change risky?]:fair [Why is the change risky/not risky?]:the change is a low risky on code level, since it just removes some code logic, but it brings some behavioral changes, which theoretically might affect on how the web content is exposed to screen readers [String changes made/needed]:no

Comment on attachment 8847730 [details] [diff] [review] patch +browser test fix Fix for regression/fairly high volume crash, includes new tests, let's uplift to beta 10.

Comment on attachment 8847730 [details] [diff] [review] patch +browser test fix Crash rate is rather high on 52, so let's take this.

Setting qe-verify- based on Alexander's assessment on manual testing needs (see Comment 17) and the fact that this fix has automated coverage.

Backed out from ESR52 for causing bug 1349847 (more to come). Resetting the tracking flag too since I'm not certain we wouldn't want to eventually take this fix again once it lands without making stability worse. https://hg.mozilla.org/releases/mozilla-esr52/rev/8067b8e306ed

Backed out from 53/54 as well. Leaving them set to affected in the event of a later uplift request. https://hg.mozilla.org/releases/mozilla-aurora/rev/fdfd001037c7 https://hg.mozilla.org/releases/mozilla-beta/rev/4d660d9d4255 https://hg.mozilla.org/releases/mozilla-release/rev/4d660d9d4255

Well, maybe. But I don't think for 53 post-release. If anyone disagrees strongly with that let me know.

Hi Alexander, Per comment #24, will you create another patch for Beta54?

(In reply to Gerry Chang [:gchang] from comment #26) > Hi Alexander, > Per comment #24, will you create another patch for Beta54? unfortunately I don't have any good guess why this bug makes the crash ratio higher. I'm hunting down for the reasons that keep us crashing, so when we get the crash fixed on nightly, then we can backport those patches, probably with this one relanded.

Comment on attachment 8847730 [details] [diff] [review] patch +browser test fix resetting approval flags since this was backed out

This is likely wontfix for 54 at this point, as there's not much time left and this is a risky area.

I've kind of lost track of which bugs fix which bugs with respect to a11y crashes. Does something need backporting to ESR52 here still or can we call this fixed?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #30) > I've kind of lost track of which bugs fix which bugs with respect to a11y > crashes. Does something need backporting to ESR52 here still or can we call > this fixed? iirc this bug helps to reduce crash ratio of bug 1321384, so it might be good idea to backport it to 52, if the crash ratio is high there. This bug needs a fix from bug 1363027 (since bug 1349847 depends on it), which seems wasn't yet backported to 52.

Bug 1363027 and bug 1349847 were uplifted to ESR52, but I'm thinking that we should leave well enough alone at this point and not uplift this unless it's blocking more important patches from landing. Feel free to set the status back to affected and nominate it if you feel strongly.

ESR52 is seeing mozilla::a11y::Accessible::SetARIAHidden crashes at a decent rate still (over 500 in the last week per crash-stats). Given that, maybe we should reconsider that wontfix.

Note: ESR 52 maybe become recommended to our accessibility users if we don't have the multi-process windows a11y work ready. It would be good to fix this.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #33) > ESR52 is seeing mozilla::a11y::Accessible::SetARIAHidden crashes at a decent > rate still (over 500 in the last week per crash-stats). Given that, maybe we > should reconsider that wontfix. should I go and request esr52 uplift (only)?

Comment on attachment 8847730 [details] [diff] [review] patch +browser test fix [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: sec issue: bug 1321384 Fix Landed on Version: 55 Risk to taking this patch (and alternatives if risky): moderate risk, check out latter discussion in bug 1321384 for more info String or UUID changes made by this patch: no

Comment on attachment 8847730 [details] [diff] [review] patch +browser test fix Fix a crash and sec issue. Let's uplift it to ESR52.3.