1274612 - Check callers have the exclusive access lock at compile time

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

There are a few places in the runtime where the exclusive access lock is required.  This is currently checked with assertions, but we can make these methods take an AutoLockForExclusiveAccess& parameter and enforce this at compile time.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: require-exclusive-lock

Require lock for JSRuntime::atoms, atomsCompartment and symbolRegistry.

Mostly straightforward, but annoyingly I had to make ComponentFinder use CRTP so we could pass around a reference to the lock with it as ZoneComponentFinder.  

Reading this, AutoLockForExclusiveAccess seems rather unwieldy.  I was wondering about changing it to something more pithy, maybe AutoLockExclusiveAccess or even just AutoLockExclusive.  What do you think?

Comment 2

[Jon Coppeard (:jonco)]

Attachment: require-exclusive-lock-2

Do the same for JSRuntime::parseMapPool, addActiveCompilation, removeActiveCompilation and scriptDataTable.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8754884 [details] [diff] [review]
require-exclusive-lock

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

This is great! I'm also in favor of shortening the lock name. Even better would be to move all of the shared state under an ExclusiveData, but that might be too big a bridge to cross at this point.

I suppose we could just make the SCC algorithm take the lock itself, given that it's only used in one place right now. I'm not sure if that's actually cleaner than the CRTP though.

::: js/src/gc/GCRuntime.h
@@ +638,5 @@
>          TraceRuntime,
>          MarkRuntime
>      };
> +    void markRuntime(JSTracer* trc, TraceOrMarkRuntime traceOrMark,
> +                     AutoLockForExclusiveAccess& lock);

Are these not const because they actually unlock or because it's too cumbersome?

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8754885 [details] [diff] [review]
require-exclusive-lock-2

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

::: js/src/vm/Runtime.cpp
@@ -430,5 @@
>      finishSelfHosting();
>  
>      MOZ_ASSERT(!exclusiveAccessOwner);
>  
> -    // Avoid bogus asserts during teardown.

O_O

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99d94b060b63

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3908a77903cf

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99d94b060b63
https://hg.mozilla.org/mozilla-central/rev/3908a77903cf