search

shaka-project / shaka-player-embedded

Shaka Player in a C++ Framework
Apache License 2.0
238 stars 63 forks source link

[JSC] BackingObjects with JS component are only cleaned up when marked as ShortLived #62

Open Halmo opened 4 years ago

Halmo commented 4 years ago

When using JavaScriptCore instead of v8 it appears that BackingObjects are not cleaned up if they have a JsThis associated, unless they are marked as ShortLived by overriding IsShortLived.

The BackingObject stores the associated JsThis using a WeakJsPtr. The WeakJsPtr implementation for JSC stores the underlying object using a Handle (v8 uses a Global ref). For JSC a Handle is a util::CFRef, when a CFRef of template type JSValueRef or JSObjectRef is constructed it calls JSValueProtect which prevents JSC from GCing that ref. Unless I'm mistaken this means that the WeakJsPtr implementation for JSC is not really a weak reference to the JS object. The protect done by the Handle/CFRef is going to prevent the object from being GC'd until the Handle/CFRef is destroyed, which never happens once one exists for a BackingObject, as far as I can tell.

When a BackingObject has an associated JsThis it returns true for IsRootedAlive (for JSC only). IsRootedAlive is used by ObjectTracker::IsJsAlive to determine if the object is still alive in JS, because of the above IsRootedAlive will always return true once a JsThis is associated with a BackingObject which means the BackingObject will never be destroyed. The only case I can see where the objects are destroyed for JSC is when IsShortLived returns true and short circuits the IsRootedAlive check.

All the above said this prevents objects such as MediaSource (which is quite heavy as it spawns a thread for the PipelineManager etc) from ever being cleaned up once instanced, even when there is no reference to it left on the JS side. If you are to load enough videos in the lifetime of SPE you will easily run out of system resources (as low as 10 video loads depending on the device).

Please let me know if I'm missing something or if this is a known limitation when using JSC with SPE. Thanks!

TheModMaker commented 4 years ago

You are correct, the WeakJsPtr is implemented as a strong pointer in JSC because I wasn't able to find a way to create traceable weak pointers in JSC.

When using V8, the v8::Global<T> can be marked as weak and we get a callback during a GC to indicate that the given object is still alive (through our Trace method).

In JSC, we have no such method. If we don't call JSValueProtect, the pointer will become a dangling pointer once the JS value is freed and we can't trace it to conditionally keep it alive (i.e. use our Trace method).

We probably can't have the JsThis method return a new object each time since then the JS objects will compare differently even though they refer to the same C++ object.

In the case of MediaSource, we may be able to mark it as IsShortLived. While the MediaSource is running, it is owned by the HTMLVideoElement, which always has a non-zero ref count by the app. So we'd trace the HTMLVideoElement and get to the MediaSource. Once the video is detached, Shaka Player doesn't use it anymore, so it is effectively short lived.

Another option I just thought of is to just store the raw pointer and not protect it. Once the JS object is destroyed, we get a callback where we can clear the pointer on the BackingObject. Technically this is invalid since properties added on the object should be preserved, even if there isn't an explicit JS reference; but I don't think we use this behavior. It doesn't matter that a new JS object is created because JS can't tell the difference (since it doesn't have a reference).

But in general, yes, this is a limitation of JSC. I haven't been able to figure out how to make a weak pointer that can be traced to conditionally be kept alive.

Halmo commented 4 years ago

Thanks for the quick response @TheModMaker. 👍 I had thought about storing the raw JSObjectRef/JSValueRef on WeakJsPtr and clearing the ref when finalize is called for the object as well. But for some reason when I made the change to store the Ref directly instead of a Handle objects were not living properly and I was getting undefined references in JS. I didn't really dig into that too much further because I wasn't sure whether I was on the right track or not. I will need to dig into this further regardless though because we wouldn't be able to have these objects leaking in a production environment.

TheModMaker commented 4 years ago

After #60, the JavaScript MediaSource object is no longer in charge of managing the media pipeline. The large media pipeline is either managed by the app or by the ShakaPlayer instance. The MediaSource and HTMLVideoElement objects may live longer than needed; but they are thin wrappers around those objects, so it shouldn't affect memory that much.

Looking some more at our heap (ObjectTracker), there are only a few JavaScript objects alive over a long time period. So I don't think this leak is that important. Especially since there may be little we can do. JavaScriptCore doesn't allow us to create real weak references, so there may be no other option.

Since this shouldn't cause any more large leaks, I'm moving this to the backlog. I'd like to remove the IsShortLived hacks, but I don't know of a way to do it.