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
Panic with Windowproxy traced while being transplanted #23959
Comments
My guess is we're not rooting enough in |
This gist has the simple script and HTML/CSS files that I used to trigger this: https://gist.github.com/kanaka/119f5ed9841e23e35d07e8944cca6aa7 |
I updated to ffb9e33 and reran the test and got the same stack trace on the 819th load so it seems consistent even across a build with a code change. |
That bit of code hasn't changed much in the last three years, so I don't think any recent changes will have affected it. It needs a very specific set of circumstances to trigger it, I'm a bit surprised we're able to reproduce it! |
My interpretation of the stack trace is that it's GC related in mozjs. Is that correct? If so, an interesting data-point is that there is no user JS code in the test page. Just a bunch of imported CSS stylesheets (actually just 10 copies of two files), one inline style on the body tag, and a bit of HTML. Another data-point (that I noted in the original issue #23905) is that the resident memory size of the servo process is increasing by about 57MB for every 100 loads of this test page. Any changes you would like me to make to the test page or the build to help debug this? |
Yes, it's caused by GC in spidermonkey being triggered in the middle of My current guess is that the window proxy needs rooted during let _rooted_self = DomRoot::from_ref(self); to the beginning of |
I applied this patch and rebuilt and still got the same crash at 819 page loads: --- a/components/script/dom/windowproxy.rs
+++ b/components/script/dom/windowproxy.rs
@@ -552,6 +552,7 @@ impl WindowProxy {
fn set_window(&self, window: &GlobalScope, traps: &ProxyTraps) {
unsafe {
debug!("Setting window of {:p}.", self);
+ let _rooted_self = DomRoot::from_ref(self);
let handler = CreateWrapperProxyHandler(traps);
assert!(!handler.is_null());
|
Oh rats. I'll see if I can come up with something else... |
Ok what did caught my attention is that we seem to be doing some redundant transplanting, and with that fixed it appears the issue goes away(or at least I wasn't able to reproduce it, altough I didn't try yet without the fix). @asajeffrey Perhaps you want to review it at #23967 ? We probably should keep this one open if the above fix doesn't exclude the possibility of not enough rooting taking place, as a separate problem exposed by the other? @kanaka maybe you want to try locally as well and see if it fixes it? You can use the branch at #23909 which contains the additional commit. |
@gterzian building it now. |
Hmm, this is just papering over the cracks though, the GC panic is still there waiting to be triggered. |
Crash on load 1237 (compared to 819 previously). Still growing at consistent 56-57MB per page load. Different stack trace (unfortunately no labels again):
|
That looks like you're back to running out of fds again (or something else is triggering weird low-level fd-adjacent errors in ipc). |
I'm experimenting with making the test case larger to try and reduce the amount of time needed to run the test. My first experiment was adding a bunch of lorem ipsum text to the test case. This increased the memory resident growth rate (108MB per load). However, the test still crashed at exactly 1273 loads as before. However, this time I got a better stack trace:
|
I increased the size one of the repeated CSS files that is included and got a trace (after 1236 loads) that is definitely more related to #23905 so I'll post the stack there. |
Here is a branch that represents the state of the tree that seemed to reliably trigger the GC crash: https://github.com/gterzian/servo/tree/trigger_windowproxy_trace_crash |
cc @asajeffrey
Originally posted by @kanaka in #23905 (comment)
The text was updated successfully, but these errors were encountered: