Closed Bug 1274612 Opened 4 years ago Closed 4 years ago

Check callers have the exclusive access lock at compile time


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox49 --- fixed


(Reporter: jonco, Assigned: jonco)



(2 files)

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.
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?
Attachment #8754884 - Flags: review?(terrence)
Do the same for JSRuntime::parseMapPool, addActiveCompilation, removeActiveCompilation and scriptDataTable.
Attachment #8754885 - Flags: review?(terrence)
Comment on attachment 8754884 [details] [diff] [review]

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?
Attachment #8754884 - Flags: review?(terrence) → review+
Comment on attachment 8754885 [details] [diff] [review]

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

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

Attachment #8754885 - Flags: review?(terrence) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.