VBufStorage: do not delete reference nodes prematurely when replacing subtrees#10188
Merged
Conversation
Member
Author
|
@MarcoZehe Would you mind testing this pr with your one or more Gmail crashes please? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… separate loop, before removing the replaced nodes from the backend. Otherwise, some referenced nodes might be prematurely deleted.
…a node must be part of the same parent to be reused. This code did not handle descendants and the true underlying cause of crashes has been dealt with in vbufStorage_buffer_t::replaceSubtrees now.
79a1db6 to
b5052f9
Compare
Contributor
|
Fix confirmed, thank you! Does this have a chance of being backported to a possible 2019.2.1 release? |
feerrenrut
suggested changes
Sep 12, 2019
…er understanding and to avoid shadowing variables from an outer scope.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Fixes #10175
Fixes #9402
Fixes #8924
Replaces pr #8930
Summary of the issue:
NVDA could crash the web browser if a page moved an accessibility node from one subtree on the page to another, either by moving a DOM node directly, or when setting aria-owns dynamically. The crash became much more likely if two nodes were swapped with each other at the same time, such as in issue #10175 .
The crash was occuring because VBufStorage_buffer_t::replaceSubtrees would move reference nodes for reuse, and then straight away remove the old subtree and replace it with the content of the temporary buffer, However reference nodes in other subtrees may be further replaced, containing their own reference nodes which may reference nodes that have already been removed.
Description of how this pull request fixes the issue:
VBufsTorage_buffer_t::replaceSubtrees now moves reference nodes on all temp buffers first, and only then removes the old subtrees and replaces them with the content of the temp buffers.
This pr also reverts code in pr #8930 which removes the limitation in VBufBackend_t::reuseExistingNodeInRender where it would refuse to return an existing node if it did not share the same parent. This particular code was flawed as it never took descendants into account. For example in issue #10175: If item1 was moved from container2 to container1, although item1 itself wouldn't be reused as the parents differ, item1's child div would be reused, therefore not avoiding a possible crash. Now the underlying issue is addressed, it is now hopefully safe enough to reuse nodes from differing parents, though we could obviously reinstate that code if we wanted.
Testing performed:
Performed testcase in #10175. Firefox no longer crashes.
Performed testcase in #9402. Firefox no longer crashes.
Performed testcase in #8924. Firefox no longer crashes.
Known issues with pull request:
None known.
Change log entry:
Addressed several crashes in Gmail seen in both Firefox and Chrome when interacting with particular popup menus such as when creating filters or changing certain Gmail settings.