Skip to content

Conversation

samdeane
Copy link
Contributor

I'm not sure if you'll want to take everything here, as there might be some Sketch specific things (I'm not certain of that, but I'm aware that I haven't had a chance to factor out all the changes nicely). There are certainly some project changes caused by upgrading to XC7, for example, and some other older debugging changes.

However, you might want to take an attempt I made to clean up the boxing, and a possible fix for the Mocha crash.

We had been operating with the "fix" that just called JSValueProtect on every object, and thus leaked all the JS objects.

I had yet another think about what the problem might be (assuming that it's not a bug in JSCore), and the theory I came up with was that either:

  • there might be situations where the JS object gets garbage collected whilst we have a reference to it stored somewhere other than our box (the prime candidate being ffi invocations)
  • or that there might be some short-circuiting going on where JSCore doesn't call finalize because it knows that we've made an object which nothing ever references

I suspect that the second situation might be happening with MOStruct. We seem to make JS objects here, and thus cause them to be boxed, but we don't actually hold on to the JS refs anywhere that I can see, other than the boxes themselves. The objects seem to be made in passing, as a way of extracting structures out of ffi return values.

My speculation is that in this situation JSCore might be "smart" enough not to bother actually initialising/finalising the object at all (or it might be a bug that it doesn't get finalised).

My fix for these two possible scenarios is to call JSValueProtect:

  • on every argument before a function/method invocation
  • on every JS object who's corresponding Obj-C object is stored in an MOStruct

I can then balance these with calls to JSValueUnprotect:

  • after the function invocation returns
  • when an MOStruct is destroyed

I'm not certain that this is a valid fix, but it seems to work in one of the cases that was crashing for me here.

It's a slightly better fix than just calling JSValueProtect everywhere, as it does allow some objects to be freed up.

I've also improved the cleanup that COScript/MochaRuntime does when shutting down. In particular, they clear out the box table, and the script removes its reference to the runtime, thus breaking a retain cycle that was happening for us (because, I think, the runtime contained a JS variable that had a reference to the script).

samdeane and others added 30 commits April 30, 2014 11:41
Unfortunately there's no other way to suppress them optionally that I can find, and they were the only warnings in our build.
Import paths can be in the format:

    relative/to/script.js
    /absolute/path/to/script.js
    ~/relative/to/home/script.js
BohemianCoding/Sketch#3319 Script import: Add support for absolute and ~ paths
Also guards against multiple imports of the same URL, and therefore circular imports

Ref BohemianCoding/Sketch#3319
BohemianCoding/Sketch#3319 Add support for nested @import statements
Fix a bug which ignored first line of @import-ed scripts
randomsequence and others added 20 commits August 20, 2015 17:12
Ref BohemianCoding/Sketch#5799
Ref BohemianCoding/Sketch#6742
Added explicit unprotection and removal of boxes in an attempt to prevent reference loops when session is cleaned up.
The theory here is that JS references to objects may be in the ffi arguments as raw pointers, so it's a good idea to protect everything. May be spurious...
…get created but aren't referenced from JS anywhere at the point the struct is made, so I suspect that they might be destroyed again immediately.
We will "leak" any object in the map until the JS reference is garbage collected, or until the end of the script session - which is not ideal, but better than crashing.
# Conflicts:
#	Cocoa Script.xcodeproj/project.pbxproj
#	src/framework/mocha/MochaRuntime.m
#	src/framework/mocha/Objects/MOBox.m
@ccgus
Copy link
Owner

ccgus commented Dec 17, 2015

Thanks for this. Looks like it's causing some issues in Acorn right now, as this bit is happening:
NSAssert((_JSObject == nil) && (_representedObject == nil), @"should have been disassociated");

Do you know offhand why that would happened? Which part of coscript should have cleaned that up?

@ccgus
Copy link
Owner

ccgus commented Dec 17, 2015

OK, answering my own question- looks like MOBoxManager assumes _index is valid and around in makeBoxForObject:jsClass:, but it isn't when it's dealloc'd. So I got that fixed.

There seems to be a lot of breakage with NSDistantObject, which Acorn uses a ton of. So until I get those figured out, I'll have to hold off on merging this.

@samdeane
Copy link
Contributor Author

I'm not surprised that there are some loose ends, as there's probably plenty of stuff we don't use. I also suspect that there may still be unbalanced protect/unprotect calls at times, but at least that just means leaking some objects rather than all.

Why would makeBox be called when it's dealloc'd? Or was that not what you meant?

@ccgus
Copy link
Owner

ccgus commented Dec 18, 2015

That's what I meant. At some point when cleaning up and in a dealloc, MOBoxManager's makeBoxForObject:jsClass: is being called, at least with one of my scripts.

This was referenced Jan 18, 2016
@samdeane
Copy link
Contributor Author

samdeane commented Jul 8, 2016

Closing this as #40 is a more up to date version of it.

@samdeane samdeane closed this Jul 8, 2016
@samdeane samdeane deleted the feature/6774-plugins-are-leaking-like-a branch November 25, 2016 11:19
afedor pushed a commit to afedor/CocoaScript that referenced this pull request Dec 20, 2020
Code Review: MSLogAction not actually called (#17688)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants