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

assertion failed: `*self.stack == mem::transmute(&*self)` in wpt #6462

Closed
Ms2ger opened this issue Jun 25, 2015 · 7 comments
Closed

assertion failed: `*self.stack == mem::transmute(&*self)` in wpt #6462

Ms2ger opened this issue Jun 25, 2015 · 7 comments

Comments

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Jun 25, 2015

Haven't investigated further.

CC @michaelwu @jdm @Manishearth

@michaelwu
Copy link
Contributor

@michaelwu michaelwu commented Jun 26, 2015

Generally this assertion gets hit when servo panics with a RootedObject/RootedValue on stack. The panic prevents the destructor for the root from running, which then breaks this particular assertion. Destructors for GC thing roots have to be run in the correct order or all hell breaks loose.

This can also happen if someone tries to return or move a RootedObject/Value without appropriate use of the new_with_addr function.

@Ms2ger
Copy link
Contributor Author

@Ms2ger Ms2ger commented Jun 26, 2015

Destructors should still run after a panic, though

@jdm
Copy link
Member

@jdm jdm commented Jul 14, 2015

I'm convinced that this is due to SpiderMonkey being compiled with -fno-exceptions (so destructors won't get run when we try to unwind through C++), as well as unwinding from Rust across FFI being undefined behaviour. The assertion appears to trigger because a C++ RootedValue is on the stack when the panic occurs, and its destructor doesn't end up getting run so our Rust destructor gets hopelessly confused. Potential ways forward include:

19:34 <jdm> consider a Window::Crash method that panics
19:34 <jdm> we're supposed to catch the panic before returning to the C++ caller of the binding method
19:34 <jdm> and then.... what?
19:34 <jdm> SM will think everything is fine
19:35 <jdm> and then we need to store the fact that we caught a panic somewhere (TLS?) and check that every time we return from any code that could end up returning to Rust code
19:35 <jdm> and re-panic

19:52 <pcwalton> this is a totally wacky idea, but could we throw a JavaScript exception to propagate the panic out?
19:52 <pcwalton> (assume we have magic JS exceptions that content can't catch)

19:54 <pcwalton> here's another weird idea: use catch_panic! and continue, but put an event in the event queue that will kill the script task
19:54 <pcwalton> that way script ends up dying once it reaches the outermost event boundary
@jdm jdm added I-panic and removed I-crash labels Jul 14, 2015
@Ms2ger
Copy link
Contributor Author

@Ms2ger Ms2ger commented Jul 14, 2015

If that's the case, the stack looks like [dead C++ object]+ [our Rust object], right? Then how about we just remove all the dead C++ objects as well?

@jdm
Copy link
Member

@jdm jdm commented Jul 14, 2015

Just keep popping until we get to the expected address, you mean? That would help with this assertion, but anything like JSAutoRequest would leave unfinished requests, etc. which will likely trigger more assertions.

@jdm
Copy link
Member

@jdm jdm commented Jan 11, 2016

rust-lang/rfcs#1413 looks like it could be useful.

@DemiMarie
Copy link

@DemiMarie DemiMarie commented Feb 1, 2016

I suspect that the best approach is to catch all panics at every in-call to Rust and kill either the thread or process immediately, or kill JavaScript execution (by returning false without raising a JS exception) and then store the panic value in TLS. This is then checked on return to Rust and propogated.

jdm added a commit to jdm/servo that referenced this issue Jun 20, 2016
…e the interrupted panic to the origin of the JS execution via TLS before resuming. Fix servo#6462.
jdm added a commit to jdm/servo that referenced this issue Jun 20, 2016
…e the interrupted panic to the origin of the JS execution via TLS before resuming. Fix servo#6462.
@jdm jdm mentioned this issue Jun 20, 2016
4 of 4 tasks complete
jdm added a commit to jdm/servo that referenced this issue Jun 20, 2016
…e the interrupted panic to the origin of the JS execution via TLS before resuming. Fix servo#6462.
jdm added a commit to jdm/servo that referenced this issue Jun 22, 2016
…e the interrupted panic to the origin of the JS execution via TLS before resuming. Fix servo#6462.
bors-servo added a commit that referenced this issue Jun 22, 2016
Avoid unwinding into C stack frames

Fix the biggest cause of #6462 by wrapping lots of JS->Rust transitions in catch_panic, and calling resume_panic after all Rust->JS transitions return.

Known issue:
* Finalizers can be called in response to any JS engine allocation that triggers a GC, so it's possible for a Rust object's Drop implementation that panics to leave an interrupted panic in TLS. This is why 30d8009 is part of this PR; the underlying problem is that there's no clear place to resume the panic after it is interrupted.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #6462 (github issue number if applicable).
- [X] There are tests for these changes OR

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11803)
<!-- 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.

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