Skip to content
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

TransitionEnd event can include nodes that have been GCed #14972

Closed
jdm opened this issue Jan 11, 2017 · 5 comments
Closed

TransitionEnd event can include nodes that have been GCed #14972

jdm opened this issue Jan 11, 2017 · 5 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jan 11, 2017

https://github.com/servo/servo/commits/5d3847dddc9bb7907abfa5d38a7927d6c656fbc1/components/layout/animation.rs sends an asynchronous message to the script thread that contains an UnsafeNode value. When the script thread processes the message, it casts the unsafe node to a DOM node and tries to use it. If the node has been GCed since the layout event that caused the message to be sent, this is a crash.

I propose fixing this in the same way that #14962 deals with asynchronous operations involving DOM nodes - collect them during layout and have the script thread retrieve them immediately to prevent risk of them being GCed.

I hit this in a debugger while running the TodoMVC benchmark: https://lhorie.github.io/todomvc-perf-comparison/todomvc-benchmark/

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jan 11, 2017

This looks like it's something that I ought to have fixed but didn't because I assumed that a layout node always have a valid reference to its underlying DOM node. Could you expand on how the "collection during layout" and "retrieving in script thread" works?

@jdm
Copy link
Member Author

@jdm jdm commented Jan 11, 2017

The easiest way I've found is to create a layout RPC mechanism to retrieve a list of nodes, and invoke that right after reflow completes in script. Meanwhile layout populates the list via a member of the various context objects that are passed around. Of course, this only works if the reflow is initiated from script. If something's else is driving the animations in layout, we need a different solution.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 12, 2017

I can often hit this on the benchmark linked above if I disable the Ractive, Likely, and Mithril suites.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 31, 2017

I'm hitting this repeatedly when creating a blank document on docs.google.com with #15082 applied (and other related document.open/document.write hacks to allow page loading to progress).

@jdm
Copy link
Member Author

@jdm jdm commented Jan 31, 2017

Blocks #13942.

bors-servo added a commit that referenced this issue Apr 7, 2017
Root nodes for the duration of their CSS transitions

This ensures that we can pass a node address as part of the asynchronous
transition end notification, making it safe to fire the corresponding
DOM event on the node from the script thread. Without explicitly rooting
this node when the transition starts, we risk the node being GCed before
the transition is complete.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14972
- [X] There are tests for these changes
bors-servo added a commit that referenced this issue Apr 10, 2017
Root nodes for the duration of their CSS transitions

This ensures that we can pass a node address as part of the asynchronous
transition end notification, making it safe to fire the corresponding
DOM event on the node from the script thread. Without explicitly rooting
this node when the transition starts, we risk the node being GCed before
the transition is complete.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14972
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16295)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Apr 10, 2017
Root nodes for the duration of their CSS transitions

This ensures that we can pass a node address as part of the asynchronous
transition end notification, making it safe to fire the corresponding
DOM event on the node from the script thread. Without explicitly rooting
this node when the transition starts, we risk the node being GCed before
the transition is complete.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14972
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16295)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 15, 2017
Root nodes for the duration of their CSS transitions

This ensures that we can pass a node address as part of the asynchronous
transition end notification, making it safe to fire the corresponding
DOM event on the node from the script thread. Without explicitly rooting
this node when the transition starts, we risk the node being GCed before
the transition is complete.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14972
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16295)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.