New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MOObjectDescription addInstanceMethodWithSelector:function:] causes crashes when argument list isn't exact #28

Open
matt-curtis opened this Issue Apr 12, 2015 · 7 comments

Comments

Projects
None yet
2 participants
@matt-curtis
Copy link

matt-curtis commented Apr 12, 2015

Calling this method behaves oddly in regards to arguments:

1.) When the JavaScript function is called, the arguments variable is only ever as populated as the predefined arguments. E.g, when assigning the function function(){ arguments } to a method selector such as "my:method:selector:" arguments.length will always equal 0, regardless of the actual parameters sent along with the message.

2.) The function specified can only have the exact same (or less) number of arguments as the selector has parameters. Any greater number of arguments causes the runtime to crash. For the selector "my:method:selector:" this function will work: function(a, b, c){} but function(a, b, c, d){ arguments } causes a crash. This breaks and defies the fluid nature of JavaScript Function arguments.

It also appears that calling this method (and it's companion addClassMethod version) can cause the JSValue and its JSContext to be retained.

@matt-curtis matt-curtis changed the title [MOObjectDescription addInstanceMethodWithSelector:function:] causes crashes when argument list isn't correct [MOObjectDescription addInstanceMethodWithSelector:function:] causes crashes when argument list isn't exact Apr 12, 2015

matt-curtis pushed a commit to matt-curtis/MochaJSDelegate that referenced this issue Apr 12, 2015

Matt Curtis
Fixed issue where arguments weren't passed to handlers
See comment, attempt to work around this Mocha issue:

logancollins/Mocha#28
@afedor

This comment has been minimized.

Copy link

afedor commented Apr 16, 2015

Do you have any stand-alone example code which illustrates the problem? It would be easier to debug if there was a nice example.

@matt-curtis

This comment has been minimized.

Copy link
Author

matt-curtis commented Apr 16, 2015

Sure - here's one I just constructed. Tested in Sketch.

COScript.currentCOScript().setShouldKeepAround_(true);

//  Create class

var uniqueClassName = "Mocha_DynamicClass_"+new Date().getTime();

var delegateClassDesc = MOClassDescription.allocateDescriptionForClassWithName_superclass_(uniqueClassName, NSObject);

var selector = "webView:didFinishLoadForFrame:";

var onLoaded = function(){
    var app = [NSApplication sharedApplication];
    [app displayDialog:"WebView Loaded!" withTitle:"Alert"];

    win.close();
    COScript.currentCOScript().setShouldKeepAround_(false);
};

//  !! -- BEGIN Crashing Code -- !!

//  This example works fine

delegateClassDesc.addInstanceMethodWithSelector_function_(selector, function(webView, webFrame){
    onLoaded();
});

//  This example works fine too, but arguments only equals [webView], instead of [webView, webFrame]

delegateClassDesc.addInstanceMethodWithSelector_function_(selector, function(webView){
    onLoaded();
});

//  This example works fine too, but arguments only equals [], instead of [webView, webFrame]

delegateClassDesc.addInstanceMethodWithSelector_function_(selector, function(){
    onLoaded();
});

//  This example crashes - anything more than the exact number of arguments crashes

delegateClassDesc.addInstanceMethodWithSelector_function_(selector, function(webView, webFrame, extraArg){
    onLoaded();
});

//  !! -- END Crashing Code -- !!

delegateClassDesc.registerClass();

//  Create Window

var frame = NSMakeRect(0, 0, 500, 400);
var styleMask = NSTitledWindowMask | NSClosableWindowMask | NSResizableWindowMask;
var win  = [[NSWindow alloc] initWithContentRect:frame styleMask:styleMask backing:NSBackingStoreBuffered defer:false];

//  Create WebView

var webView = WebView.new();

webView.setFrameLoadDelegate_(NSClassFromString(uniqueClassName).new());
webView.setMainFrameURL_("http://google.com");

webView.setFrame_(frame);
win.contentView().addSubview_(webView);

//  Show Window

win.makeKeyAndOrderFront_(NSApp);
win.center();
@afedor

This comment has been minimized.

Copy link

afedor commented Apr 17, 2015

Well, unfortunately, this appears to be a tough nut to crack. When the delegate method implementation is called, there doesn't appear to be any information we can use to find the correct number of arguments. The block implementation provides the object, but NOT the selector (surprise!), and the venerable C varargs list does not give any information about how many arguments are provided.

Possibly the only work around would be to keep some global list of function to selector mappings. That's pretty ugly though.

@matt-curtis

This comment has been minimized.

Copy link
Author

matt-curtis commented Apr 17, 2015

Maybe I'm not understanding, but wouldn't this solve that?

@afedor

This comment has been minimized.

Copy link

afedor commented Apr 17, 2015

Nope. That's why I put in the little "surprise!" text. You might look at http://landonf.bikemonkey.org/code/objc/imp_implementationWithBlock.20110413.html to understand why that is. i.e. _cmd does not exist in the block implementation that Mocha uses to generate the class methods

@matt-curtis

This comment has been minimized.

Copy link
Author

matt-curtis commented Apr 17, 2015

Ah, OK. I reviewed the source more thoroughly and that article and I see now...what a bother, haha. va_list makes this more hopeless... (I also just realized my 'fix' in my library won't handle va_list...awesome.)

OK, here's another route: what about using forwardInvocation: and [NSInvocation getArgument:atIndex:] somehow? I realize this would mean using a shim class to catch the invocation then forwarding it to the actual class or something...just spitballing here. This is an interesting problem

@afedor

This comment has been minimized.

Copy link

afedor commented Apr 17, 2015

Well your still left with having to store the mapping between the function and the selector somewhere - although in this case you could probably put it in the shim class itself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment