Skip to content
This repository has been archived by the owner on Dec 7, 2018. It is now read-only.

Prototope, Xcode, and Protocaster all crash when you forget “new” in your script #25

Open
jbrennan opened this issue Feb 18, 2015 · 6 comments
Labels

Comments

@jbrennan
Copy link
Contributor

This one was fun. I had the following line in my script,

var layer = TextLayer()

And I couldn’t figure out what was wrong with it. Turns out I forgot new. Anyway, when Prototope hit that line, it crashed, and sometimes this caused Xcode to crash (betas!). Obviously the script is wrong but for some reason Prototope didn’t catch the exception...not quite sure why yet.

@jbrennan jbrennan added the bug label Feb 18, 2015
@saniul
Copy link
Contributor

saniul commented Mar 6, 2015

Investigated a bit – looks like the constructor functions that get created for the JSExported classes are generated in such a weird way that they crash JavaScriptCore when called straight, without the new keyword. If you look at the JSExport.h documentation, it actually states that those constructors aren’t
callable:

screen shot 2015-03-06 at 00 16 47

This is an actual crash (not a thrown exception) in JavaScriptCore, couldn’t figure out a workaround :( We can either live with it or stop using the constructor and expose a create method on all bridged types. I think just making sure we always write new is a better option, despite the crashiness.

@andymatuschak
Copy link
Contributor

Yikes. I've got no bright ideas here, other than e.g. preprocessing the JS to look for this kind of mistake, which is probably overboard. @jbrennan?

@kevinbarabash
Copy link
Contributor

It's probably not a bad idea to do some sort of linting before running. If we're looking to support JavaScript as a language it shouldn't look too different from regular JavaScript that you see in the browser/node. For instance, I was surprised that there was no setTimeout, but instead there's a afterDuration function. Hopefully we can support new as well as more of the JavaScript standard library in the future.

@NachoSoto
Copy link
Contributor

How do we define the constructors? I don't know if this is possible because I haven't really looked at the bridging code, but you can detect in a constructor if it's been invoked correctly by checking if (this instanceof Constructor)

@saniul
Copy link
Contributor

saniul commented Mar 6, 2015

Constructor objects are defined by JavaScriptCore automatically based on the exposed init in the XXXJSBridge protocol (the one that extends JSExport).

Sent from my iPhone

On 6 Mar 2015, at 09:23, NachoSoto [email protected] wrote:

How do we define the constructors? I don't know if this is possible because I haven't really looked at the bridging code, but you can detect in a constructor if it's been invoked correctly by checking if (this instanceof Constructor)


Reply to this email directly or view it on GitHub.

@sophiebits
Copy link
Contributor

Just seeing this in passing via email so I haven't looked at how the code is set up, but if you can run something along these lines before the user code, perhaps it will help:

function wrapNativeClass(OriginalConstructor) {
  function Constructor() {
    if (!(this instanceof Constructor)) {
      throw new Error("Classes should be instantiated with `new`");
    }
    OriginalConstructor.apply(this, arguments);
  }
  Constructor.prototype = OriginalConstructor.prototype;
  return Constructor;
}

TextLayer = wrapNativeClass(TextLayer);

Constructor.prototype.constructor will still be OriginalConstructor which may be a problem? If so, you can probably copy things over from the prototype manually. At the very least, you probably want to make sure that new TextLayer() instanceof TextLayer is still true.

sophiebits added a commit to sophiebits/Prototope that referenced this issue Mar 7, 2015
For Khan#25: This shows how we might wrap the constructors so that they throw when called without `new`. In this example, I've hardcoded `TextLayer` as the only wrapped type, but we'd presumably want to wrap every instantiable type. When you call a type without `new`, you now get a protonope, as you might expect. (It always says line 5 though since that's where the `new Error()` call is in the wrap script -- not sure how to fix that easily.)
sophiebits added a commit to sophiebits/Prototope that referenced this issue Mar 7, 2015
For Khan#25: This shows how we might wrap the constructors so that they throw when called without `new`. In this example, I've hardcoded `TextLayer` as the only wrapped type, but we'd presumably want to wrap every instantiable type. When you call a type without `new`, you now get a protonope, as you might expect. (It always says line 5 though since that's where the `new Error()` call is in the wrap script -- not sure how to fix that easily.)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants