Assertion failure: init(mozilla::Forward<F>(f), mozilla::Forward<Args>(args)...), at js/src/threading/Thread.h:110

RESOLVED FIXED in Firefox 51

Status

()

--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gkw, Assigned: jandem)

Tracking

(Blocks: 1 bug, {assertion, jsbugmon, testcase})

Trunk
mozilla52
x86_64
Mac OS X
assertion, jsbugmon, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
The following testcase crashes on mozilla-central revision da986c9f1f72 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

for (var j = 0; j < 2999; ++j) {
    evalInWorker("(function(){});timeout(1);");
}


Backtrace:

0   libsystem_kernel.dylib        	0x00007fff84d0d10a __semwait_signal + 10
1   libsystem_pthread.dylib       	0x00007fff887f7787 pthread_join + 444
2   js-dbg-64-dm-clang-darwin-da986c9f1f72	0x0000000108576de9 js::Thread::join() + 25 (Thread.cpp:117)
3   js-dbg-64-dm-clang-darwin-da986c9f1f72	0x00000001084b8606 main + 13334 (js.cpp:3321)
4   js-dbg-64-dm-clang-darwin-da986c9f1f72	0x00000001084b26b4 start + 52
/snip

For detailed crash information, see attachment.

Probably a threading issue, but GC is all over the stack so setting s-s as a start.
(Reporter)

Comment 1

2 years ago
Created attachment 8799070 [details]
Detailed Crash Information
(Reporter)

Comment 2

2 years ago
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160528055932" and the hash "17d8abb5b1803418ea206cd60fe5386dc8dded98".
The "bad" changeset has the timestamp "20160528071135" and the hash "132e391ed5e588241227dfb48083dcd57ac3142f".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=17d8abb5b1803418ea206cd60fe5386dc8dded98&tochange=132e391ed5e588241227dfb48083dcd57ac3142f
(Reporter)

Comment 3

2 years ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8aa6b27297b6
user:        Terrence Cole
date:        Wed May 25 09:41:57 2016 -0700
summary:     Bug 956899 - Use js::Thread for JS shell watchdog thread; r=jandem

Jan, is bug 956899 a likely regressor? (Also setting needinfo? from Terrence in case he has time for this)
Blocks: 956899
Flags: needinfo?(terrence.d.cole)
Flags: needinfo?(jdemooij)
(Assignee)

Comment 4

2 years ago
Not s-s, shell-only and safe crash.
Group: javascript-core-security
(Assignee)

Comment 5

2 years ago
Created attachment 8799356 [details] [diff] [review]
Patch

js::Thread has a fallible init() method. It also has a constructor that calls init() and MOZ_RELEASE_ASSERTs it returns true. That assert is failing here for the watchdog thread.

This patch removes the constructor that calls init. It seems clearer to have each caller call init() explicitly and deal with OOM appropriately.

The only places I had to fix were the watchdog thread (this crash) and some jsapi-tests. The tests are slightly more verbose now, but it's test-only so I will take that over potential footguns.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(terrence.d.cole)
Flags: needinfo?(jdemooij)
Attachment #8799356 - Flags: review?(nfroyd)
Comment on attachment 8799356 [details] [diff] [review]
Patch

Review of attachment 8799356 [details] [diff] [review]:
-----------------------------------------------------------------

Should have realized that infallible constructors were a bad idea when I reviewed this code originally.  Thanks for cleaning this up.
Attachment #8799356 - Flags: review?(nfroyd) → review+

Comment 7

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8efe890d2a28
Remove js::Thread's infallible constructor. r=froydnj

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8efe890d2a28
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Reporter)

Comment 9

2 years ago
Are we able to easily backport this issue to mozilla-aurora? If so, that would be nice.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 10

2 years ago
Comment on attachment 8799356 [details] [diff] [review]
Patch

Requesting approval to help fuzz testing. I think this is mostly NPOTB.

Approval Request Comment
[Feature/regressing bug #]: Older bug.
[User impact if declined]: Crashes when fuzzing.
[Describe test coverage new/current, TreeHerder]: Fixes tests, on m-c for a while.
[Risks and why]: Very low risk. It shouldn't affect the browser at all.
[String/UUID change made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #8799356 - Flags: approval-mozilla-aurora?

Updated

2 years ago
status-firefox51: --- → affected
Comment on attachment 8799356 [details] [diff] [review]
Patch

Fix a fuzzing assertion error. Take it in 51 aurora.
Attachment #8799356 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/179f83a4ec89
status-firefox51: affected → fixed
You need to log in before you can comment on or make changes to this bug.