onclick changes shouldn't recreate the tree

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 fixed, firefox53- wontfix, firefox54+ wontfix, firefox55+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch patch (obsolete) — Splinter Review
let's keep things shorter
Assignee: nobody → surkov.alexander
Attachment #8847120 - Flags: review?(yzenevich)
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
Attachment #8847120 - Flags: review?(yzenevich) → review+
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 -
Flags: needinfo?(surkov.alexander)
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.
Attachment #8847120 - Attachment is obsolete: true
Flags: needinfo?(surkov.alexander)
Attachment #8847730 - Flags: review?(yzenevich)
Comment on attachment 8847730 [details] [diff] [review]
patch +browser test fix

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

thanks
Attachment #8847730 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/mozilla-central/rev/3873a60605b3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Attachment #8847730 - Flags: approval-mozilla-aurora?
Comment on attachment 8847730 [details] [diff] [review]
patch +browser test fix

This patch helps investigate the crash and include tests. Aurora54+.
Attachment #8847730 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1346518
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
Flags: needinfo?(surkov.alexander)
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
Flags: needinfo?(surkov.alexander)
Attachment #8847730 - Flags: approval-mozilla-beta?
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.
Attachment #8847730 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8847730 [details] [diff] [review]
patch +browser test fix

Crash rate is rather high on 52, so let's take this.
Attachment #8847730 - Flags: approval-mozilla-esr52+
Setting qe-verify- based on Alexander's assessment on manual testing needs (see Comment 17) and the fact that this fix has automated coverage.
Flags: qe-verify-
Depends on: 1349847
See Also: → 1350888
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
Well, maybe. But I don't think for 53 post-release. If anyone disagrees strongly with that let me know.
Depends on: 1350888
See Also: 1350888
Hi Alexander,
Per comment #24, will you create another patch for Beta54?
Flags: needinfo?(surkov.alexander)
(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.
Flags: needinfo?(surkov.alexander)
Comment on attachment 8847730 [details] [diff] [review]
patch +browser test fix

resetting approval flags since this was backed out
Attachment #8847730 - Flags: approval-mozilla-esr52+
Attachment #8847730 - Flags: approval-mozilla-beta+
Attachment #8847730 - Flags: approval-mozilla-aurora+
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?
Flags: needinfo?(surkov.alexander)
(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.
Flags: needinfo?(surkov.alexander)
Depends on: 1376754
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.
Depends on: 1376825
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
Attachment #8847730 - Flags: approval-mozilla-esr52?
Comment on attachment 8847730 [details] [diff] [review]
patch +browser test fix

Fix a crash and sec issue. Let's uplift it to ESR52.3.
Attachment #8847730 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.