js::Thread is used after engine shutdown

RESOLVED FIXED in Firefox 58

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
19 days ago
12 days ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

19 days ago
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.
Jon, can you take this?
status-firefox58: --- → affected
Flags: needinfo?(jcoppeard)
Priority: -- → P1
(Assignee)

Comment 2

14 days ago
Sure.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 3

14 days ago
Created attachment 8926467 [details] [diff] [review]
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.
Attachment #8926467 - Flags: review?(luke)
(Assignee)

Comment 4

14 days ago
Created attachment 8926470 [details] [diff] [review]
bug1414310-mach-exception-handler

Patch to destroy the mach exception handing thread during shutdown rather than when global data is destroyed.
Attachment #8926470 - Flags: review?(jdemooij)
(Assignee)

Comment 5

14 days ago
Created attachment 8926473 [details] [diff] [review]
bug1414310-shell-off-thread-state

Patch to destroy shell off thread compilation state (which includes a mutex in js::Monitor) before shutdown.
Attachment #8926473 - Flags: review?(sphink)
(Assignee)

Comment 6

14 days ago
Created attachment 8926474 [details] [diff] [review]
bug1414310-shell-buffer-stream-state

Patch to destroy shell buffer stream state before shutdown.
Attachment #8926474 - Flags: review?(sphink)
Comment on attachment 8926470 [details] [diff] [review]
bug1414310-mach-exception-handler

Ah, I had no idea that js::Thread has this requirement/limitation.
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.
Attachment #8926473 - Flags: review?(sphink) → review+
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.
Attachment #8926474 - Flags: review?(sphink) → review+

Comment 10

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

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

Good idea
Attachment #8926467 - Flags: review?(luke) → review+
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.
Attachment #8926470 - Flags: review?(jdemooij) → review+

Comment 12

13 days ago
Pushed by jcoppeard@mozilla.com:
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

12 days ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 12 days ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.