This ended up boiling more of the ocean than I'd hoped for, but the resulting code is much nicer.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7152cba5644c

For some reason `bool js::Thread::init<void (&)(JSRuntime*), JSRuntime*&>(void (&&&)(JSRuntime*), JSRuntime*&&&)` is calling vanilla operator new. > 000000000068c560 <__ZN2js6Thread4initIRFvP9JSRuntimeEJRS3_EEEbOT_DpOT0_>: > <snip> > 68c5d7: e8 b6 cc a0 00 callq 1099292 <__Znwm$stub> > <snip>

Inside `js::Thread::init`: > auto trampoline = new Trampoline(mozilla::Forward<F>(f), > mozilla::Forward<Args>(args)...); Oh. That should be js_new.

Waldo says that this is on purpose so that we can lift js::Thread to mfbt/mozglue. We should either (1) fix check-vanilla-allocations to ignore js::Thread::init (2) make js::Thread parameterized by an alloc policy, similar to mozilla::Vector I don't have strong opinions either way.

Terrence, happen to have an opinion on comment 5? I'm leaning towards (2) because I think it will be easier (the check-vanilla-allocations script doesn't have any understanding of who is calling who, only what symbols end up in the object file) and is also more correct, in that we can continue avoiding use of the system allocator like the rest of js/ does and be consistent.

Err, forgot to ni -- see comment 3 through comment 6.

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #5) > Waldo says that this is on purpose so that we can lift js::Thread to > mfbt/mozglue. > > We should either > > (1) fix check-vanilla-allocations to ignore js::Thread::init > (2) make js::Thread parameterized by an alloc policy, similar to > mozilla::Vector > > I don't have strong opinions either way. Or (3) we could make it js_new for now and whoever moves it to mfbt can change that line?

Comment on attachment 8775790 [details] [diff] [review] Make js::HelperThread::thread a js::Thread instead of a PRThread Review of attachment 8775790 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/HelperThreads.cpp @@ +152,5 @@ > } > } > > /* Wait for in progress entries to finish up. */ > + for (HelperThread& helper : *HelperThreadState().threads) { Let's use auto& here to be consistent with the other iterations.

Try push with review fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28d57ab7197c&group_state=expanded

Pushed by nfitzgerald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/14c1777276fe Make js::HelperThread::thread a js::Thread instead of a PRThread; r=terrence

This or something else from the push appears to have turned 10.10 debug cpp tests permared: https://treeherder.mozilla.org/logviewer.html#?job_id=33028238&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/63a40cee73f3

It ended up being this patch that caused that failure. We were doing a `MOZ_RELEASE_ASSERT` on the allocation of the thread's trampoline but that doesn't play well with `js_new`. I just made it an `AutoOOMUnsafeRegion` to keep the old semantics, even though it seems like we could maybe get away with handling the failure. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=710c4a0ee5ed

Pushed by nfitzgerald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/60cd2460476f Make js::HelperThread::thread a js::Thread instead of a PRThread; r=terrence