Do not trace Rust values when thread is shutting down. #18952

jdm · 2017-10-19T14:27:20Z

This addresses a paint point when using debug-mozjs builds. jonco says that it is considered a leak when objects stored in side tables in a SpiderMonkey embedding are traced right before shutting down. This PR adds a per-thread flag that controls whether to run the Rust-side trace hooks, which is automatically toggled before the final GC occurs when destroying a JS runtime.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #18948 and fix #18947.
These changes do not require tests because we don't use debug-mozjs on CI

This change is

highfive · 2017-10-19T14:27:24Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/bindings/refcounted.rs, components/script/dom/workerglobalscope.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/worklet.rs, components/script/script_thread.rs and 3 more
@KiChjang: components/script/dom/bindings/refcounted.rs, components/script/dom/workerglobalscope.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/worklet.rs, components/script/script_thread.rs and 3 more

highfive · 2017-10-19T14:27:25Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2017-10-19T14:33:23Z

I am optimistic that these changes fix a bunch of other crashes that have been reported by nightly users.

jdm · 2017-10-19T14:34:50Z

r? @nox

jdm · 2017-10-19T14:34:54Z

@bors-servo: try

bors-servo · 2017-10-19T14:34:59Z

⌛ Trying commit b0f3a97 with merge 58b7391...


        Auto merge of #18952 - jdm:no-leak-on-shutdown, r=<try>

Do not trace Rust values when thread is shutting down. This addresses a paint point when using debug-mozjs builds. jonco says that it is considered a leak when objects stored in side tables in a SpiderMonkey embedding are traced right before shutting down. This PR adds a per-thread flag that controls whether to run the Rust-side trace hooks, which is automatically toggled before the final GC occurs when destroying a JS runtime. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18948 and fix #18947. - [x] These changes do not require tests because we don't use debug-mozjs on CI  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18952)

bors-servo · 2017-10-19T15:35:29Z

☔ The latest upstream changes (presumably #18944) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2017-10-19T16:30:08Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

nox · 2017-10-20T15:56:45Z

@bors-servo r+

bors-servo · 2017-10-20T15:56:45Z

📌 Commit 6f66b0c has been approved by nox

bors-servo · 2017-10-20T16:09:42Z

⌛ Testing commit 6f66b0c with merge 7bef051...


        Auto merge of #18952 - jdm:no-leak-on-shutdown, r=nox

Do not trace Rust values when thread is shutting down. This addresses a paint point when using debug-mozjs builds. jonco says that it is considered a leak when objects stored in side tables in a SpiderMonkey embedding are traced right before shutting down. This PR adds a per-thread flag that controls whether to run the Rust-side trace hooks, which is automatically toggled before the final GC occurs when destroying a JS runtime. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18948 and fix #18947. - [x] These changes do not require tests because we don't use debug-mozjs on CI  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18952)

bors-servo · 2017-10-20T16:18:10Z

💔 Test failed - linux-rel-css


        Do not trace Rust values when thread is shutting down.

jdm · 2017-10-20T17:50:56Z

@bors-servo: r=nox

bors-servo · 2017-10-20T17:50:57Z

📌 Commit c075372 has been approved by nox

bors-servo · 2017-10-20T18:49:01Z

⌛ Testing commit c075372 with merge 1d61cb6...


        Auto merge of #18952 - jdm:no-leak-on-shutdown, r=nox

Do not trace Rust values when thread is shutting down. This addresses a paint point when using debug-mozjs builds. jonco says that it is considered a leak when objects stored in side tables in a SpiderMonkey embedding are traced right before shutting down. This PR adds a per-thread flag that controls whether to run the Rust-side trace hooks, which is automatically toggled before the final GC occurs when destroying a JS runtime. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18948 and fix #18947. - [x] These changes do not require tests because we don't use debug-mozjs on CI  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18952)

bors-servo · 2017-10-20T19:44:09Z

⛄ The build was interrupted to prioritize another pull request.

bors-servo · 2017-10-20T20:55:10Z

⌛ Testing commit c075372 with merge e542e69...


        Auto merge of #18952 - jdm:no-leak-on-shutdown, r=nox

Do not trace Rust values when thread is shutting down. This addresses a paint point when using debug-mozjs builds. jonco says that it is considered a leak when objects stored in side tables in a SpiderMonkey embedding are traced right before shutting down. This PR adds a per-thread flag that controls whether to run the Rust-side trace hooks, which is automatically toggled before the final GC occurs when destroying a JS runtime. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18948 and fix #18947. - [x] These changes do not require tests because we don't use debug-mozjs on CI  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18952)

bors-servo · 2017-10-20T21:47:27Z

💔 Test failed - linux-rel-wpt

jdm · 2017-10-20T21:52:40Z

@bors-servo: retry

#15388

bors-servo · 2017-10-21T01:28:05Z

⌛ Testing commit c075372 with merge a71470a...


        Auto merge of #18952 - jdm:no-leak-on-shutdown, r=nox

Do not trace Rust values when thread is shutting down. This addresses a paint point when using debug-mozjs builds. jonco says that it is considered a leak when objects stored in side tables in a SpiderMonkey embedding are traced right before shutting down. This PR adds a per-thread flag that controls whether to run the Rust-side trace hooks, which is automatically toggled before the final GC occurs when destroying a JS runtime. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18948 and fix #18947. - [x] These changes do not require tests because we don't use debug-mozjs on CI  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18952)

bors-servo · 2017-10-21T03:14:40Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: nox
Pushing a71470a to master...

highfive assigned wafflespeanut Oct 19, 2017

highfive added the S-awaiting-review label Oct 19, 2017

jdm force-pushed the jdm:no-leak-on-shutdown branch from 221acba to b0f3a97 Oct 19, 2017

highfive assigned nox and unassigned wafflespeanut Oct 19, 2017

highfive added the S-needs-rebase label Oct 19, 2017

jdm force-pushed the jdm:no-leak-on-shutdown branch from b0f3a97 to 6f66b0c Oct 19, 2017

jdm removed the S-needs-rebase label Oct 19, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 20, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 20, 2017

Do not trace Rust values when thread is shutting down.

Loading status checks…

c075372

jdm force-pushed the jdm:no-leak-on-shutdown branch from 6f66b0c to c075372 Oct 20, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Oct 20, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 20, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 20, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 21, 2017

bors-servo merged commit c075372 into servo:master Oct 21, 2017
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Oct 21, 2017

jdm mentioned this pull request Nov 27, 2017

System freeze after trying to load techcrunch.com #17932

Closed

servo / servo

Do not trace Rust values when thread is shutting down. #18952

Do not trace Rust values when thread is shutting down. #18952

jdm commented Oct 19, 2017 •

edited

highfive commented Oct 19, 2017

highfive commented Oct 19, 2017

jdm commented Oct 19, 2017

jdm commented Oct 19, 2017

jdm commented Oct 19, 2017

bors-servo commented Oct 19, 2017

bors-servo commented Oct 19, 2017

bors-servo commented Oct 19, 2017

nox commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

jdm commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

jdm commented Oct 20, 2017

bors-servo commented Oct 21, 2017

bors-servo commented Oct 21, 2017

servo / servo

Join GitHub today

Do not trace Rust values when thread is shutting down. #18952

Do not trace Rust values when thread is shutting down. #18952

Conversation

jdm commented Oct 19, 2017 • edited

highfive commented Oct 19, 2017

highfive commented Oct 19, 2017

jdm commented Oct 19, 2017

jdm commented Oct 19, 2017

jdm commented Oct 19, 2017

bors-servo commented Oct 19, 2017

bors-servo commented Oct 19, 2017

bors-servo commented Oct 19, 2017

nox commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

jdm commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

bors-servo commented Oct 20, 2017

jdm commented Oct 20, 2017

bors-servo commented Oct 21, 2017

bors-servo commented Oct 21, 2017

jdm commented Oct 19, 2017 •

edited