992256 - Assertion failure: !isLocked(), at js/src/jsworkers.cpp:479

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Logical locking/asserting problem

in GlobalWorkerThreadState::ensureInitialized()
> jsworkers.cpp:420 AutoLockWorkerThreadState lock;
> ...
> jsworkers.cpp:437 threads[j].destroy();

in WorkerThread::destroy()
> jsworkers.cpp:694 if (threads) {
> jsworkers.cpp:696     AutoLockWorkerThreadState lock;

As a result we are asserting in the destroy function that we already have the lock.
I needinfo Brian since that code was last changed in bug 941805 by Brian.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Good catch.  Worker thread initialization only happens once near process startup so there's really no incentive to try to recover from OOM here.

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 8404752 [details] [diff] [review]
patch

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

However this patch would fix the issue, I don't see the reason to introduce a crash statement? 
I think we can just fix this issue without removing the fallibility of ensureInitialized, with using the normal way to propagate errors.
Wouldn't it work to have a "destroyAlreadyHavingLock" which has the meat of destroy() and asserts we indeed have a lock?
the "destroy" funtion would in that case just do "AutoLockWorkerThreadState lock;" and call "destroyAlreadyHavingLock();"?

Comment 3

[Brian Hackett [Laid off!]]

Doing that would work, but it would be more complicated and difficult to test (seeing as how fuzzers didn't find this bug).  I don't think we should have complicated OOM handling logic unless doing otherwise will cause crashes in practice, e.g. see bug 988619.  Per comment 1 the only way these calls will fail is if we run out of memory around when we start up the process, which means we're pretty much doomed anyways.

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8404752 [details] [diff] [review]
patch

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

Although I would prefer to have less of these CrashAtUnhandlableOOM("FOO");. It makes sense. Thanks for fixing!

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ab932f4d41

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2ab932f4d41