1448091 - [e10s a11y] Node removals aren't reflected in JAWS virtual buffer

Status: RESOLVED FIXED

Description

[James Teh [:Jamie]]

Attachment: Test case.

STR (with JAWS):
1. Open the attached test case.
2. Press the Go button.
3. Read through the text of the document using the arrow keys.
Expected: The text "This paragraph will be removed." should not be present.
Actual: It is.

If you disable the COM handler (by setting the pref accessibility.handler.enabled to false and restarting), this does not occur. So, this is somehow related to AccessibleHandler. However, this does not occur in either case with NVDA.

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1448091 part 1: Accessible HandlerProvider: Return an error if refreshing the cache fails.

BuildDynamicIA2Data can't return an HRESULT; failure is communicated by the fact that the unique id in the data is set to 0.
However, IGeckoBackChannel::Refresh returned S_OK even if BuildDynamicIA2Data failed.
We now check the unique id and return an appropriate error code if it's 0.
Among other things, this means that the handler will return an error when querying dead objects instead of returning stale cache data.

Review commit: https://reviewboard.mozilla.org/r/230660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230660/

Comment 2

[James Teh [:Jamie]]

Attachment: Bug 1448091 part 2: AccessibleHandler: Don't repeatedly and unnecessarily refresh the cache after the first change.

The singleton AccessibleHandlerControl maintains a global cache generation counter which is incremented whenever any node is changed.
The handler for each node tracks the cache generation at the time its cache was initialized.
To check whether the cache needs to be updated, the handler compares its generation against the global generation from AccessibleHandlerControl.
If they differ, it refreshes the cache.
Previously, the handler didn't update its cache generation after refreshing.
This meant that every single query after the first change would refresh the whole cache, even if there wasn't a second change.
This resulted in a huge number of wasted cache updates for clients such as JAWS which hold onto accessibles, rather than releasing them and fetching them again for each update.

Review commit: https://reviewboard.mozilla.org/r/230666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230666/

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8961841 [details]
Bug 1448091 part 2: AccessibleHandler: Don't repeatedly and unnecessarily refresh the cache after the first change.

https://reviewboard.mozilla.org/r/230666/#review236146

::: accessible/ipc/win/handler/AccessibleHandler.cpp:223
(Diff revision 1)
> -  return mCachedData.mGeckoBackChannel->Refresh(&mCachedData.mDynamicData);
> +  HRESULT hr =  mCachedData.mGeckoBackChannel->Refresh(&mCachedData.mDynamicData);
> +  if (SUCCEEDED(hr)) {
> +    // We just updated the cache, so update this object's cache generation
> +    // so we only update the cache again after the next change.
> +    mCacheGen = gen;
> +  }

if you failed to update the cache, then do you still need to keep old version or should you instead to nullify it?

Comment 4

[James Teh [:Jamie]]

Some notes on these patches:
Part 1 is the fix I expected to work for this bug. Curiously, it does fix this test case, but it sometimes doesn't work for the same case in larger documents. I don't really understand why.
Part 2 seems to fix this even in larger documents. Again, I don't really understand why. I suspect there is some kind of timing issue here, but I can't work out exactly what it is.
Part 2 also fixes extreme slowness when refreshing documents with JAWSKey+escape.
Even though there are some unknowns here, both of these patches correct behaviour which is very clearly incorrect. I'm inclined to accept that "correctness" deals with this problem rather than spending a huge amount of time on figuring out the exact interactions in a complex system.

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 8961840 [details]
Bug 1448091 part 1: Accessible HandlerProvider: Return an error if refreshing the cache fails.

https://reviewboard.mozilla.org/r/230660/#review236154

Comment 6

[Marco Zehe (:MarcoZ)]

Comment on attachment 8961841 [details]
Bug 1448091 part 2: AccessibleHandler: Don't repeatedly and unnecessarily refresh the cache after the first change.

https://reviewboard.mozilla.org/r/230666/#review236156

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/702ffbc7ccd5
part 1: Accessible HandlerProvider: Return an error if refreshing the cache fails. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/a61e917f9406
part 2: AccessibleHandler: Don't repeatedly and unnecessarily refresh the cache after the first change. r=MarcoZ

Comment 8

[Raul Gurzau (:RaulGurzau)]

https://hg.mozilla.org/mozilla-central/rev/702ffbc7ccd5
https://hg.mozilla.org/mozilla-central/rev/a61e917f9406

Comment 9

[Marco Zehe (:MarcoZ)]

Verified fixed in Firefox 61.0a1 (20180324100127).

Comment 10

[James Teh [:Jamie]]

Comment on attachment 8961840 [details]
Bug 1448091 part 1: Accessible HandlerProvider: Return an error if refreshing the cache fails.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1303060/e10s Windows a11y.
[User impact if declined]: Incorrect content presented to screen reader users when nodes are removed from the document.
[Is this code covered by automated tests?]: No; no mechanism to test platform a11y.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: Other patch from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Straightforward correctness patch.
[String changes made/needed]: None.

Comment 11

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #10)
> [Needs manual test from QE? If yes, steps to reproduce]:

Sorry. The answer to this is "no".

Comment 12

[James Teh [:Jamie]]

Comment on attachment 8961841 [details]
Bug 1448091 part 2: AccessibleHandler: Don't repeatedly and unnecessarily refresh the cache after the first change.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1303060/e10s Windows a11y.
[User impact if declined]: Incorrect content presented to screen reader users when nodes are removed from the document.
[Is this code covered by automated tests?]: No; no mechanism to test platform a11y.
[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]: Other patch from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Straightforward correctness patch.
[String changes made/needed]: None.

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8961841 [details]
Bug 1448091 part 2: AccessibleHandler: Don't repeatedly and unnecessarily refresh the cache after the first change.

a11y fix for beta60.  should be in 60.0b8.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/50c5d93932ca
https://hg.mozilla.org/releases/mozilla-beta/rev/880bdfaff98e

Comment 15

[Marco Zehe (:MarcoZ)]

Verified fixed in Firefox 60.0b8.