Use the String(string) form of string coercion

[sorentwo]

Using string.toString() will throw errors in current versions of Safari
for some values. The error is a particularly cryptic "Type Error: type
error", which no indication as to the value that caused the error. By
using the String(string) form of coercion the error doesn't seem to
occur.

There is about an inherent performance penalty ranging from 2-42%,
depending on the browser: http://jsperf.com/string-coercion-comparison

[kpdecker]

What are the values that fail? Can you provide a fiddle and Safari versions that this fails on? I'm hesitant to opt into something that will impact the performance for core logic like escapeExpression without fully understanding the problem that it is solving.

[sorentwo]

After a full hour and a half trying to recreate the bug in a controlled environment I'm going to throw my hands up and say I am stumped. In our application with real data locally I can recreate it every time. When I attempt to replicate it with a jsfiddle it works without any errors.

I'll put all the relevant data that I can, in the hopes that somebody recognizes some aspect of the problem that I am missing.

This is the real template from the app (it is hamlbars). The if/else is necessary because of how haml approaches values. It isn't possible to do %option({{#if isSelected}}selected{{/if}})

%select
  %option(value = '0') Select Group
  {{#each missionGroups}}
  {{#if isSelected}}
  %option(value = '{{id}}' selected) {{name}}
  {{else}}
  %option(value = '{{id}}') {{name}}
  {{/if}}
  {{/each}}

The data being rendered looks like this:

[
  { isSelected: false, id: 101, name: "Trials" },
  { isSelected: true,  id: 102, name: "Something" },
  { isSelected: false, id: 103, name: "Something POV" }
]

When that runs in Safari with the console closed it will yield the error TypeError: Type error at the line string = string.toString(); within Utils.escapeExpression. If the console is open or you use any other browser the error doesn't occur. This is with Safari Version 6.0.4 (8536.29.13), not sure about others.

Manipulating the template in other ways also prevented the problem entirely. Here are some of the things I tried, all of which prevent the error:

1. Add a random {{log "something"}} statement at the top of the each. It doesn't matter what the log value is.
2. Remove either {{id}} or {{name}}, both will work.
3. Use the unescape form, {{{name}}}, which works for somewhat obvious reasons.

Here is the fiddle where I attempted to recreate the failure scenario. I'd love to hear if anybody has an idea what may be going on here. In the meantime I can fully understand not wanting to pull this in!

[kpdecker]

We have seen issues in the past that I believe were related to a bug in Safari's JIT compiler but the fix was a bit of an unknown one. #440 discusses this a little bit but I'm not sure that helps for this case.

[sorentwo]

Considering how easy it is to mitigate the issue (using any type of log statement), that it is isolated to a specific version of Safari, and the performance degradation I'm going to go ahead and close this.

If anything it is good to have it documented where others may find it.

[kpdecker]

Sounds good. Thanks!

On Wed, Jun 5, 2013 at 9:24 AM, Parker Selbert notifications@github.comwrote:

Considering how easy it is to mitigate the issue (using any type of log
statement), that it is isolated to a specific version of Safari, and the
performance degradation I'm going to go ahead and close this.

If anything it is good to have it documented where others may find it.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/535#issuecomment-18978864
.

[GeReV]

Just a note, this issue persists in Safari 6.0.5 as well.

[sorentwo]

@GeReV Have you been able to mitigate it through the use of {{log "anything}}?

[GeReV]

@sorentwo, I actually went straight to your pull request due to lack of time to update every template I have.
The pull request did fix the issue.

[watterssn]

I am receiving this error as well. Strangely enough it doesn't happen if the console is open.

[sorentwo]

I am receiving this error as well. Strangely enough it doesn't happen if the console is open.

@watterssn That is part of what makes it really tricky to find. I essentially had to binary chop the page until I found the template(s) causing it.

[kpdecker]

That this occurs when the console is closed makes it REALLY sound like a JIT issue. If we can reduce this to a test case that we can send this over to Apple to try to get it fixed upstream.

@GeReV @watterssn this PR fixes all of the issues? If you can reproduce on a regular basis a repo case that I can work with would be best but otherwise could you try individual changes here? I'm very suspicious of the toString implementation being the only associated issue given the JIT theory and it's simplicity and presumed hotpath.

[sorentwo]

@kpdecker I have a consistently failing example in my main app, but have been unable to get it to reproduce in isolatio One possible difference is that in the jsfiddle the full Handlebars library is loaded, while in production we are only loading the runtime.

[IvanTorresEdge]

We also ran into this issue and we use handlebars in production in conjunction with Backbone.js+Marionette. The issue is when we pass a model to the render some values are numbers and cause the error (escapeExpression). This happens under the same conditions mentioned above, Safari 6.0.5 with the developer console closed.

So in our case there's only two options:
a) We would need to stringify every model (serializeData)
b) We patch Handlebars (using type coercion "" + string, which seems to be faster than String(string) http://jsperf.com/convert-to-string-bj/3)

Option a is too much work. Has anyone come up with a better solution? We really hope this or another pull request gets merged.

[sorentwo]

I've rebased off of master and switched to the "" + string method of coercion. It is almost always faster, other than on Firefox where it is comparable. The commit message has been amended for the changes, you know, so that it makes sense.

[IvanTorresEdge]

Thanks Parker (@sorentwo)!

[kpdecker]

@sorentwo can you merge master into your branch so it diffs only your code?

I'm still hesitant on this as the perf numbers don't seem to look good, in fact worse even.
http://jsperf.com/string-coercion-comparison/3

Given that there is performance impact on any change here I would like to simplify to only the changes that are necessary. Could someone who can reproduce try applying this patch piecemeal to see if there are specific changes that fix the issue vs. not?

[watterssn]

For me, using the unescaped form {{{ }}} in just one place ANYWHERE in the template solves the problem.

I have multiple variables and as long as just one of them is surrounded by an {{{ }}} vs {{ }} the page will load without problems.

I have not tried the other solutions listed above.

[sorentwo]

@kpdecker Hmm, it looks like the other performance comparison may have been too much of a micro benchmark.

Here is a mashup of the bench output for the different versions. In this case master is .toString():

ops/msec    master         '' + string     String(string)   WINNER
--------------------------------------------------------------------------
string      12539 ±35 (6)  12607 ±36 (7)   12564 ±36 (7)    '' + string
variables   3400 ±11 (5)   3833 ±12 (6)    3351 ±9 (4)      '' + string
object      762 ±4 (4)     819 ±3 (4)      798 ±4 (4)       '' + string
array       132 ±3 (3)     128 ±3 (5)      129 ±3 (5)       master

Within actual Handlebars templates it looks like there is a performance gain from avoiding .toString().

[kpdecker]

@sorentwo humm that is certainly interesting. Leads me to believe that it's a JIT issue in the escapeExpression with the instanceof check somehow being killed off. For the sake for testing, what happens if you arbitrarily create Handlebars.SafeString instance elsewhere in your app?

@sorentwo Could you cleanup the merge and I'll take a look again? Performance numbers always make me a bit leery as they could change when other optimizations occur so I'd love to know why one thing fails vs. the other but I have less apprehension after the numbers you just posted.

BTW I plan on creating a performance benchmark that is integrated with Travis if it works out as I hope we can leave these discussions to CI :)

[kpdecker]

First comment above was directed at @watterssn :)

[watterssn]

@kpdecker running the following arbitrarily in my code throws the type error on the second line (when the console is initially closed):

console.log new Handlebars.SafeString('foo')
console.log Handlebars.Utils.escapeExpression('foo')

[watterssn]

@kpdecker actually it only blows up if I run Handlebars.Utils.escapeExpression('foo') before I call render with the handlebars template. If I render without using a handlebars template with that line it prints to console fine.

render: ->
console.log new Handlebars.SafeString('foo')
console.log Handlebars.Utils.escapeExpression('foo')
@$el.html @template(name: 'blah')
this

vs

render: ->
console.log new Handlebars.SafeString('foo')
console.log Handlebars.Utils.escapeExpression('foo')
@$el.html "asdasdf"
this

[sorentwo]

@kpdecker There should only be one commit in the diff. All clean now.

Even without the fix aimed at Safari there seems to be a compelling reason to change the escape coercion anyhow.

[kpdecker]

@watterssn It feels like we are so close to having a simplfied test case. Baring that does it happen if you do not have the safe string call? I.e. if you do template + log escape but no log safestring?

[watterssn]

@kpdecker correct. Seems to only be an issue with Handlebars.Utils.escapeExpression('foo')

[sorentwo]

@kpdecker, @watterssn Would you prefer to see the patch only cover Handlebars.Utils.escapeExpression? At this point it seems to me like the change is worth it in all 3 places.

[kpdecker]

This is running the + "" operation twice as it calls the toString implementation in the diff above.

[sorentwo]

I can see how it seems redundant. I haven't been able to dig up the commit where the apparent duplication was originally implemented, but it has been here a while. Testing with the additional coercion removed.

[kpdecker]

@sorentwo I'm generally fine with this in light of the performance numbers, outside of the one comment I just made.

I really don't like not understanding why something is failing but I fear actually understanding this requires deep knowledge of the Nitro JIT given the behavior that we have seen.

If you fixup that one comment (assuming that still resolves the issue....) I'll merge it in.

[sorentwo]

@kpdecker Updated with the redundant coercion removed. The change seems to have no impact, tests still pass and the benchmark is about on par.

[kpdecker]

Thanks!

[kpdecker]

Released in 1.1.0

[kpdecker]

@sorentwo we were apparently seeing this as well in our logs for some users and this fixed it. Many thanks! Now to figure out the TypeError: Type error message we have in preventDefault and stopPropagation :( (Unrelated to handlebars project)