New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VBufStorage: Fix crash by not allowing the reusing of nodes with differing parents. #8930

Merged
merged 2 commits into from Nov 9, 2018

Conversation

Projects
None yet
3 participants
@michaelDCurran
Contributor

michaelDCurran commented Nov 9, 2018

Link to issue number:

Closes #8924

Summary of the issue:

A web author can move a node from one place to another in the DOM. A web author can also move an accessible from one place to another within the accessibility tree, by dynamically changing aria-owns.
With the merging of pr #8678 this can in some cases cause NVDA to crash the web browser if NVDA's virtualBuffer for the document tries to reusethe moved node, as two separate updates may try to touch the same thing.
Examples of this are in #8924 including a real-life testcase in Gmail, and a specific code fragment.

Description of how this pull request fixes the issue:

NVDA's virtualBuffer code now refuses to reuse an existing node when re-rendering a parent if the existing node has a different parent to what is being re-rendered.
However, it still allows nodes to change ordering within a given parent (if the existing node does not directly disallow this, such as in tables). If we do not keep this, the German wikipedia page becomes as slow as it used to be in NVDA 2018.3.

Testing performed:

Tested the speed of the German wikipedia page. Tested the speed of moving up and down the inbox in Gmail.
Tested the code fragment in #8924.
However, waiting on @MarcoZehe to report if the try build in #8924 successfully fixes the crash for him.
I was never able to reproduce the crash.

Known issues with pull request:

None.

Change log entry:

None needed.

@michaelDCurran michaelDCurran requested a review from jcsteh Nov 9, 2018

@jcsteh

jcsteh approved these changes Nov 9, 2018

However, it still allows nodes to change ordering within a given parent (if the existing node does not directly disallow this, such as in tables). If we do not keep this, the German wikipedia page becomes as slow as it used to be in NVDA 2018.3.

When you say change ordering, I assume you mean this includes adding or removing another node? If so, changing that would probably defeat the whole point of the speed-ups, no? I guess we can't tell the difference at this point between a node being added between two existing nodes (e.g. acb) and two nodes switching places (e.g. ba)? (Where a, b and c are chronologically ordered.)

@@ -272,6 +276,18 @@ VBufStorage_controlFieldNode_t* VBufBackend_t::reuseExistingNodeInRender(VBufSto
LOG_DEBUG(L"Existing node refuses to be reused");
return nullptr;
}
// We only allow reusing nodes that remain at the same position in the tree.
// I.e. We never reuse a node that is from a different parent.

This comment has been minimized.

@jcsteh

jcsteh Nov 9, 2018

Contributor

Even though you mention parent here, the term "position" is used, which might make someone think this also applies to movement within the same parent. You could either just shorten this to talk about the same parent or clarify by saying that movement within the same direct parent is still okay.

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Nov 9, 2018

@MarcoZehe has confirmed this fixes the crashes he was experiencing.

@michaelDCurran michaelDCurran merged commit 244ed4e into master Nov 9, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2018.4 milestone Nov 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment