Isolate::low_memory_notification fails to lock Isolate

[Fayti1703]

Background

V8 uses a locking system (via v8::Locker) to ensure Isolates can only be used from one thread at a time.
A lot of methods (the headers define it as "every method below this notice") have an API contract requiring that the Isolate is locked by the current thread before they are called. This includes Isolate::LowMemoryNotification.

The Problem

mini_racer does not lock the isolate prior to calling LowMemoryNotification.

mini_racer/ext/mini_racer_extension/mini_racer_extension.cc

Lines 962 to 970 in b7fd92e

	static VALUE rb_isolate_low_memory_notification(VALUE self) {
	IsolateInfo* isolate_info;
	TypedData_Get_Struct(self, IsolateInfo, &isolate_type, isolate_info);
	if (current_platform == NULL) return Qfalse;
	isolate_info->isolate->LowMemoryNotification();
	return Qnil;
	}

With a debug build of V8, this crashes the process when exiting the garbage collector, since it (correctly) detects that the current thread is not the owning thread of the V8 heap.

With a release build, this may cause data races, particularly if one thread is running JavaScript code while another thread calls low_memory_notification on that same isolate

Solutions

Normally I would simply slap a Locker guard { isolate_info->isolate } prior to the call and open a PR, but I'm not sure that is the correct solution here.

In particular, doing so would cause any rb-thread calling low_memory_notification to block until other threads using the isolate have unlocked it (for JavaScript execution, this means the script has finished running).

Unfortunately V8 does not provide a try_lock-esque API that allows taking an Isolate lock without blocking.

There is a method for interrupting running script execution (Isolate::RequestInterrupt), but callbacks registered only fire during script execution. Using it would therefore make low_memory_notification useless while the isolate is not in use, which appears to be an intended use-case for the method.

There is an Isolate::IsInUse method, which would theoretically permit checking if another thread is currently running JavaScript or performing some other operation on the isolate; however this method is also within the "requires locking" set.

In addition, using it without locking might result in its own data race, where the thread calling low_memory_notification checks IsInUse, receives false and is then interrupted by a thread starting long-running JavaScript execution. The LMN-thread would try to acquire a lock on the Isolate and block, as per the simple solution.

(discovered while digging through #283's problems)

[lloeki]

Using it would therefore make low_memory_notification useless while the isolate is not in use, which appears to be an intended use-case for the method.

Correct. The original idea behind this is to repeatedly run short-lived scripts and get results, but they might accumulate garbage. In between, when memory usage gets too high, trigger libv8 GC via low mem notification.

In that specific use case this fits the bill:

In particular, doing so would cause any rb-thread calling low_memory_notification to block until other threads using the isolate have unlocked it (for JavaScript execution, this means the script has finished running).

So it seems like in that case, which is the only one where mini_racer makes use of low_memory_notification, the following would be fine:

Normally I would simply slap a Locker guard { isolate_info->isolate } prior to the call

I guess the following bit of code (notably the @eval_thread check) was supposed to sort of ensure that on the Ruby side but it's not sufficient anymore and doesn't cater for whatever libv8 is doing internally:

mini_racer/lib/mini_racer.rb

Lines 321 to 322 in b7fd92e

	if !@eval_thread && ensure_gc_after_idle_seconds < now - @last_eval
	@ensure_gc_mutex.synchronize do

The two debug+release cases you described plus the lack of any other feature that would fit the bill seem to make it clear that v8 intents us to just use the lock guard, so let's do that.

[lloeki]

I've done just that in ac520dd (#284), thanks a lot!

[tisba]

Not 100% sure if the original issue got 100% fixed, but it looks like it (in c195239), right, @lloeki?

[lloeki]

Yeah, I rebased so the commit sha changed to the one you mentioned but that's the one.

[tisba]

Fixed/Addressed in c195239, released as v0.12.0