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

Use BHM to terminate hanging JS threads when closing window #27027

Closed
gterzian opened this issue Jun 22, 2020 · 3 comments
Closed

Use BHM to terminate hanging JS threads when closing window #27027

gterzian opened this issue Jun 22, 2020 · 3 comments

Comments

@gterzian
Copy link
Member

@gterzian gterzian commented Jun 22, 2020

Follow-up on #27016

Since the constellation has a channel available to the BHM(s), when exiting a pipeline fails due to a hanging ScriptThread because of long-running JS, we could send a message to the BHM(there is either one, or one per content-process), so that it can terminate the thread(or even the whole process), perhaps after having sampled it.

I think this is especially important in multiprocess mode, since otherwise JS could simply keep running in a separate process after the main browser process would have exited, and there would not be a way for the user to know about it unless it would inspect running processes in some way(?).

This would have to be done in a per-platform basis, for example for Mac it seems the thread_terminate mach function could be used.

See components/background_hang_monitor.

@jdm
Copy link
Member

@jdm jdm commented Jun 22, 2020

We could also use the slow script feature to avoid having to write per-platform code - there's an interrupt callback the JS engine can call which could check a thread-local value to stop running whatever script is currently executing.

@gterzian gterzian mentioned this issue Jun 22, 2020
0 of 5 tasks complete
@gterzian
Copy link
Member Author

@gterzian gterzian commented Jun 22, 2020

https://searchfox.org/mozilla-central/source/dom/ipc/ProcessHangMonitor.cpp#53

From Gecko:

/*
 * Basic architecture:
 *
 * Each process has its own ProcessHangMonitor singleton. This singleton exists
 * as long as there is at least one content process in the system. Each content
 * process has a HangMonitorChild and the chrome process has one
 * HangMonitorParent per process. Each process (including the chrome process)
 * runs a hang monitoring thread. The PHangMonitor actors are bound to this
 * thread so that they never block on the main thread.
 *
 * When the content process detects a hang, it posts a task to its hang thread,
 * which sends an IPC message to the hang thread in the parent. The parent
 * cancels any ongoing CPOW requests and then posts a runnable to the main
 * thread that notifies Firefox frontend code of the hang. The frontend code is
 * passed an nsIHangReport, which can be used to terminate the hang.
 *
 * If the user chooses to terminate a script, a task is posted to the chrome
 * process's hang monitoring thread, which sends an IPC message to the hang
 * thread in the content process. That thread sets a flag to indicate that JS
 * execution should be terminated the next time it hits the interrupt
 * callback. A similar scheme is used for debugging slow scripts. If a content
 * process or plug-in needs to be terminated, the chrome process does so
 * directly, without messaging the content process.
 */

We could do something similar, although I'm wondering how often the callback is triggered when a script is hanging. In the case of trying to close a window it might still appear as a long hang until the next callback. Better than nothing, and could provide some infra for any "slow script" type of workflow. Also cross-platform from the start.

Also the constellation doesn't necessarily have to wait until the callback has been triggered, the window could be closed, while leaving any content-process running with a flag set so that when the callback is triggered the Js is stopped...

@gterzian
Copy link
Member Author

@gterzian gterzian commented Jun 23, 2020

Ok so that callback isn't called by SM on it's own when a script is running for a long time(I've tried), one needs to call JS_RequestInterruptCallback(cx) manually, which can be done from any thread.

// Any thread can call requestInterrupt() to request that this thread
// stop running.

https://github.com/servo/mozjs/blob/aabcc9ba889b2755f1e4e83f28323a60415a790f/mozjs/js/src/vm/JSContext.h#L854

So we need to pass the JsContext to the hang monitor, which can then request a callback to be triggered on the main thread, and then the script can be made to exit by returning false from that callback.

See for example https://searchfox.org/mozilla-central/source/dom/ipc/ProcessHangMonitor.cpp#171

bors-servo added a commit that referenced this issue Jun 29, 2020
Ensure clean shutdown of JS threads

<!-- Please describe your changes on the following line: -->

FIX #26685
FIX #26996
FIX #9672
FIX #27027

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jun 29, 2020
Ensure clean shutdown of JS threads

<!-- Please describe your changes on the following line: -->

FIX #26685
FIX #26996
FIX #9672
FIX #27027

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jun 29, 2020
Ensure clean shutdown of JS threads

<!-- Please describe your changes on the following line: -->

FIX #26685
FIX #26996
FIX #9672
FIX #27027

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jun 30, 2020
Ensure clean shutdown of JS threads

<!-- Please describe your changes on the following line: -->

FIX #26685
FIX #26996
FIX #9672
FIX #27027

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
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.