Javascript exceptions in keyhandler functions are not caught

[pascalw]

Consider the following snippet:

var handler = Phoenix.bind('down', ctrlAltCmd, function() {
  foo.bar(); // foo is not defined
});

This does not trigger any exception handling, so you won't notice anything went wrong.

Taking a quick peek at the code I noticed that here a new context is created to execute the key handlers function. This new context has no exceptionHandler set.

Is there any specific reason a new context is created here? Executing the function in the parent context or sharing the exception handler would fix this issue. Please let me know which option you think is best and I'll create a PR.

[kasper]

@pascalw Ah, darn! Good catch! The reason for the new context is that it seems to be the only way to get garbage collection to work properly with a function callback. Without a new context, all objects created within the callback would (seemingly) never get released according to Instruments. This would result in memory being leaked.

I think sharing the exception handler is the best option to go with.

[pascalw]

@kasper are you sure that would cause memory leaks? I played around a bit with Instruments but couldn't find any leaks.

Using the same JSContext is much easier so that IMO would be preferable over sharing the exception handler. I'll create a PR with that change so perhaps you can check on your end if you can find any memory leaks with this approach.

[kasper]

@pascalw I’m pretty sure, I used almost a week to figure out what is causing memory to be overly consumed.

Try this, create for example new PHApps with App.focusedApp() in the callback. Invoke the handler hundreds of times. Look for the object allocations and releases in Instruments and wait. The instances never get a dealloc call and are not released. With each invoke, more and more memory is consumed. You can observe this from Instruments as well. However, you are correct that for some reason they do not register as traditional memory leaks.

Now if you create a new context for the callback, all the objects created within the function are released “immediately” after the execution if they are not captured outside of the scope.

[pascalw]

@kasper was just messing around with this and found some interesting behaviour to say the least.
I'm simulating many calls to a keyhandler like so:

for(int i = 0 ; i < 10 ; i++) {

    for(int j = 0 ; j < 1000 ; j++) {
        PHKeyHandler *keyHandler = self.keyHandlers[@([PHKeyHandler hashForKey: @"x" modifiers:@[@"ctrl", @"alt", @"cmd"]])].nonretainedObjectValue;
        [keyHandler call];
    }

    [NSThread sleepForTimeInterval: 2];
}

The keyhandler called is just a simple function with only Window.focusedWindow().focus();. On master this results in multiple gigabytes of memory usage while the loop is running. After it finishes most of the memory is released, but some 200MB of memory is still used.

With my change memory usage peaks at about 180MB. However after the loop finishes none of the memory allocated is released.

I don't know exactly what to think of this but my guess would be that with the code on master allocating all those JSContext objects is very expensive. Those instances are properly garbage collected so that explains the drop in memory. However after having run this loop both branches consume roughly the same amount of memory which makes me think that reusing the JSContext is not an issue but apparently there is some other issue causing quite a bit of memory to be consumed.

Now I've been looking for docs on Javascriptcore and memory management but unfortunately those are quite scarce. Also my Objc skills are a bit rusty and I'm not familiar with ARC at all. If you want we could setup a short Hangout or Screenhero session so we can have a look at this together. You can find my email in my profile.

[kasper]

@pascalw Thanks for taking such care in exploring this! You are right that most likely creating extensive amounts of JSContexts is expensive. Obviously, in real life you never invoke the handlers consecutively so many times that you would notice this before the memory is already deallocated. Maybe some caching could be useful. Clearly there is still an issue, if some memory is retained.

Unfortunately, the JavaScriptCore documentation and headers are very obscure — especially related to memory management. This is what I’ve figured out: JavaScriptCore is still garbage collected even inside an ARC environment. This is why you need to use managed JSValues to ensure memory is released when needed. From my understanding, the allocations created within the context of the function callback are retained until the entire function is released. Obviously, this only happens when the handler is released and in Phoenix this happens rarely — basically only on context reloads. This is why technically they are not leaks and are not caught by Instruments as such. Clearly this is not desired and the allocations should be released when the callback is completed. I experimented with this a lot and the only way I found out to release the scope was to create a new JSContext for it. This way all allocations that are not stored outside of the function’s scope get released after the function is completed. I even contacted Apple technical developer support to get a comment about this, but they were not helpful at all. Maybe contacting the WebKit community could be useful as JavaScriptCore is part of the open-source project. I also spent some time earlier skimming through its source code to try to get an idea why the memory management behaves this way. I could’t find anything useful.

If you have time to debug this more or have any additional ideas, I’m happy to investigate them. We can definitely also have a short Google Hangout about this topic later this week or next week, if you feel it could be helpful.

[kasper]

In addition, I definitely think that sharing the context should be the way to go and this is how this was originally implemented. It also feels the most logical way. I guess it may be possible that the memory retain cycle comes entirely from somewhere else and this is only a consequence of it...

[pascalw]

What struck me as odd when I upgraded from Phoenix 1.x to 2.x is that you need all keyhandlers to be retained in Javascript.

I think this could actually be the source of retain cycles because you keep a reference to the keyhandler in Javascript while the keyhandler in turn keeps a reference to the callback function. I wonder if we could get rid of the need to retain the keyhandlers in Javascript? That would be more convenient and also possibly solve the retain cycle.

[kasper]

Nope, that is a requirement by JavaScriptCore. Phoenix 1.x used a block to incorrectly retain the callback functions which lead to a strong retain cycle. Plain Objective-C objects should not be retained as JSValues. See the link below.

In Phoenix 2.x we use JSManagedValues to retain the handlers. By specification (https://developer.apple.com/library/ios/documentation/JavaScriptCore/Reference/JSManagedValue_Ref/), a JSManagedValue must be referenced on the JavaScript side to keep it alive. It also makes sense, since now you as the user can have full control when the objects are needed. I spent quite some time in trying to research about this to get it right. :)

[kasper]

Also, context reloads can happen so rarely that we cannot rely on that to flush the memory. We cannot really know when to release the handlers without giving the control to the user.

[kasper]

To explain it shortly, a JSManagedValue retains the callback function as long as the function is referenced inside the JavaScript context. It’s specifically designed to avoid retain cycles. Basically it bridges ARC and garbage collection — the JavaScript side keeps a strong reference to the function and a JSManagedValue keeps a weak reference to the function.

[kasper]

@pascalw Sorry this took so long. I finally managed to really delve into this. Indeed, you were right with your assumptions. Creating a new context for the callback is indeed memory intensive, although it ensured all allocations created in the callback were released immediately.

What I didn’t notice previously in my profiling is that the garbage collection seems to be rather delayed in JavaScriptCore. The objects were released eventually even with the same context after a certain amount of allocations. They persist until this (magical) threshold is reached. I wish the documentations would be more detailed about how memory management is controlled in JavaScriptCore. I presume the overhead you noticed in your tests was objects that were still in queue to be released eventually.

In any case, using the same context is indeed the correct approach and now the exceptions are also handled properly. This is now fixed in master and will be released in the next release. Thanks!

[pascalw]

Awesome 👍🏻