1414310 - js::Thread is used after engine shutdown

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jon Coppeard (:jonco)]

I found in bug 1410132 that we have some use of js::Thread after JS_Shutdown() has been called.  We should fix this and assert that no one is calling these APIs before initialisation or after shutdown of the JS engine.

Comment 1

[Jason Orendorff [:jorendorff]]

Jon, can you take this?

Comment 2

[Jon Coppeard (:jonco)]

Sure.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1414310-init-assertions

Add an internal version of JS_IsInitialized that we can use to check threading APIs are not used before initialization or after shutdown.  This returns false after shutdown (unlike JS_IsInitialized) and also returns true during initialization.

Add assertions in mutex and threading code.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1414310-mach-exception-handler

Patch to destroy the mach exception handing thread during shutdown rather than when global data is destroyed.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1414310-shell-off-thread-state

Patch to destroy shell off thread compilation state (which includes a mutex in js::Monitor) before shutdown.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1414310-shell-buffer-stream-state

Patch to destroy shell buffer stream state before shutdown.

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 8926470 [details] [diff] [review]
bug1414310-mach-exception-handler

Ah, I had no idea that js::Thread has this requirement/limitation.

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8926473 [details] [diff] [review]
bug1414310-shell-off-thread-state

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

I am starting to wonder if it would be better to replace all of these ScopeExit things with runtimeMain(), contextMain(), etc.

  main() {
    create runtime;
    status = runtimeMain(rt);
    destroy runtime;
  }

  runtimeMain() {
    create context and associated things
    status = contextMain(cx);
    destroy cx;
  }

But it's not a big deal, we may as well continue on for now.

Comment 9

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8926474 [details] [diff] [review]
bug1414310-shell-buffer-stream-state

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

::: js/src/shell/js.cpp
@@ +8935,5 @@
> +        return 1;
> +    auto shutdownBufferStreams = MakeScopeExit([] {
> +        ShutdownBufferStreams();
> +        js_delete(bufferStreamState);
> +    });

Oh. The previous thing didn't work? Or it interacted badly with your changes?

r=me regardless. Whatever works.

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8926467 [details] [diff] [review]
bug1414310-init-assertions

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

Good idea

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8926470 [details] [diff] [review]
bug1414310-mach-exception-handler

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

::: js/src/vm/Initialization.cpp
@@ +109,5 @@
>      js::gc::InitMemorySubsystem(); // Ensure gc::SystemPageSize() works.
>  
>      RETURN_IF_FAIL(js::jit::InitProcessExecutableMemory());
>  
> +    RETURN_IF_FAIL(js::MemoryProtectionExceptionHandler::install());

Good catch.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b86c252b68fa
Assert that JS threading APIs are only used between JS_Initialize and JS_Shutdown r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/d41ed6f635b6
Destroy mach exception handler thread on JS_ShutDown r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b993d1dbe5f
Destroy shell off thread state before calling JS_ShutDown r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/94d45591fe60
Destroy shell buffer stream state before calling JS_ShutDown r=sfink

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/b86c252b68fa
https://hg.mozilla.org/mozilla-central/rev/d41ed6f635b6
https://hg.mozilla.org/mozilla-central/rev/7b993d1dbe5f
https://hg.mozilla.org/mozilla-central/rev/94d45591fe60