Skip to content

Commit

Permalink
Fixes: Safari's 'double finally on error bug'
Browse files Browse the repository at this point in the history
Bug Demo: http://jsbin.com/usoquy/1/edit
known to be an issue in:
* Safari 6.0.2
* iOS 6 mobile safari
* maybe others

Turns out certain versions of safari 6.0.2 and
potentially other webkits, will execute finally twice,
if an error in finally is thrown. Interestingly the
error will only be thrown once. But the code up until
the error is thrown, will be executed twice.

Although strange, it is rare during normal ideal execution
that this causes real issues, but when it does, it
is quite frustrating to debug.

Two new utility methods provide the 'safe' workaround:

Ember.tryFinally(tryable, finalizer);
Ember.tryCatchFinally(tryable, catchable, finalizer);

These helpers try to abide by the languages try/catch/finally
semantics.
  • Loading branch information
stefanpenner committed Dec 23, 2012
1 parent 77df9b4 commit 917da11
Show file tree
Hide file tree
Showing 8 changed files with 363 additions and 45 deletions.
17 changes: 9 additions & 8 deletions packages/ember-metal/lib/events.js
Expand Up @@ -211,11 +211,10 @@ function suspendListener(obj, eventName, target, method, callback) {
actions[actionIndex] = action; // replace the shared object with our copy
}

try {
return callback.call(target);
} finally {
if (action) { action[3] = undefined; }
}
function tryable() { return callback.call(target); }
function finalizer() { if (action) { action[3] = undefined; } }

return Ember.tryFinally(tryable, finalizer);
}

/**
Expand Down Expand Up @@ -258,13 +257,15 @@ function suspendListeners(obj, eventNames, target, method, callback) {
}
}

try {
return callback.call(target);
} finally {
function tryable() { return callback.call(target); }

function finalizer() {
for (i = 0, l = suspendedActions.length; i < l; i++) {
suspendedActions[i][3] = undefined;
}
}

return Ember.tryFinally(tryable, finalizer);
}

/**
Expand Down
16 changes: 11 additions & 5 deletions packages/ember-metal/lib/instrumentation.js
@@ -1,3 +1,5 @@
require('ember-metal/utils'); // Ember.tryCatchFinally

/**
The purpose of the Ember Instrumentation module is
to provide efficient, general-purpose instrumentation
Expand Down Expand Up @@ -90,17 +92,21 @@ Ember.Instrumentation.instrument = function(name, payload, callback, binding) {

var beforeValues = [], listener, i, l;

try {
function tryable(){
for (i=0, l=listeners.length; i<l; i++) {
listener = listeners[i];
beforeValues[i] = listener.before(name, time(), payload);
}

ret = callback.call(binding);
} catch(e) {
return callback.call(binding);
}

function catchable(e){
payload = payload || {};
payload.exception = e;
} finally {
}

function finalizer() {
for (i=0, l=listeners.length; i<l; i++) {
listener = listeners[i];
listener.after(name, time(), payload, beforeValues[i]);
Expand All @@ -111,7 +117,7 @@ Ember.Instrumentation.instrument = function(name, payload, callback, binding) {
}
}

return ret;
return Ember.tryCatchFinally(tryable, catchable, finalizer);
};

Ember.Instrumentation.subscribe = function(pattern, object) {
Expand Down
8 changes: 2 additions & 6 deletions packages/ember-metal/lib/observer.js
@@ -1,6 +1,6 @@
require('ember-metal/core');
require('ember-metal/platform');
require('ember-metal/utils');
require('ember-metal/utils'); // Ember.tryFinally
require('ember-metal/accessors');
require('ember-metal/array');

Expand Down Expand Up @@ -114,11 +114,7 @@ Ember.endPropertyChanges = function() {
*/
Ember.changeProperties = function(cb, binding){
Ember.beginPropertyChanges();
try {
cb.call(binding);
} finally {
Ember.endPropertyChanges();
}
Ember.tryFinally(cb, Ember.endPropertyChanges, binding);
};

/**
Expand Down
46 changes: 23 additions & 23 deletions packages/ember-metal/lib/run_loop.js
@@ -1,7 +1,7 @@
require('ember-metal/core'); // Ember.Logger
require('ember-metal/watching'); // Ember.watch.flushPending
require('ember-metal/observer'); // Ember.beginPropertyChanges, Ember.endPropertyChanges
require('ember-metal/utils'); // Ember.guidFor
require('ember-metal/utils'); // Ember.guidFor, Ember.tryFinally

/**
@module ember-metal
Expand Down Expand Up @@ -103,6 +103,10 @@ RunLoop.prototype = {
invoke(item.target, item.method, item.args);
}

function tryable() {
forEach.call(queue, iter);
}

Ember.watch.flushPending(); // make sure all chained watchers are setup

if (queueName) {
Expand All @@ -116,11 +120,8 @@ RunLoop.prototype = {
if (log) { Ember.Logger.log('Begin: Flush Sync Queue'); }

Ember.beginPropertyChanges();
try {
forEach.call(queue, iter);
} finally {
Ember.endPropertyChanges();
}

Ember.tryFinally(tryable, Ember.endPropertyChanges);

if (log) { Ember.Logger.log('End: Flush Sync Queue'); }

Expand Down Expand Up @@ -148,11 +149,8 @@ RunLoop.prototype = {
if (log) { Ember.Logger.log('Begin: Flush Sync Queue'); }

Ember.beginPropertyChanges();
try {
forEach.call(queue, iter);
} finally {
Ember.endPropertyChanges();
}

Ember.tryFinally(tryable, Ember.endPropertyChanges);

if (log) { Ember.Logger.log('End: Flush Sync Queue'); }
} else {
Expand Down Expand Up @@ -214,14 +212,17 @@ Ember.RunLoop = RunLoop;
@return {Object} return value from invoking the passed function.
*/
Ember.run = function(target, method) {
var ret, loop;
var loop,
args = arguments;
run.begin();
try {
if (target || method) { ret = invoke(target, method, arguments, 2); }
} finally {
run.end();

function tryable() {
if (target || method) {
return invoke(target, method, args, 2);
}
}
return ret;

return Ember.tryFinally(tryable, run.end);
};

var run = Ember.run;
Expand Down Expand Up @@ -261,12 +262,11 @@ Ember.run.begin = function() {
*/
Ember.run.end = function() {
Ember.assert('must have a current run loop', run.currentRunLoop);
try {
run.currentRunLoop.end();
}
finally {
run.currentRunLoop = run.currentRunLoop.prev();
}

function tryable() { run.currentRunLoop.end(); }
function finalizer() { run.currentRunLoop = run.currentRunLoop.prev(); }

Ember.tryFinally(tryable, finalizer);
};

/**
Expand Down
118 changes: 118 additions & 0 deletions packages/ember-metal/lib/utils.js
Expand Up @@ -406,3 +406,121 @@ Ember.tryInvoke = function(obj, methodName, args) {
return obj[methodName].apply(obj, args || []);
}
};

// https://github.com/emberjs/ember.js/pull/1617
var needsFinallyFix = (function() {
var count = 0;
try{
try { }
finally {
count++;
throw new Error('needsFinallyFixTest');
}
} catch (e) {}

return count !== 1;
})();

/**
Provides try { } finally { } functionality, while working
around Safari's double finally bug.
@method tryFinally
@for Ember
@param {Function} function The function to run the try callback
@param {Function} function The function to run the finally callback
@param [binding]
@return {anything} The return value is the that of the finalizer,
unless that valueis undefined, in which case it is the return value
of the tryable
*/

if (needsFinallyFix) {
Ember.tryFinally = function(tryable, finalizer, binding) {
var result, finalResult, finalError;

binding = binding || this;

try {
result = tryable.call(binding);
} finally {
try {
finalResult = finalizer.call(binding);
} catch (e){
finalError = e;
}
}

if (finalError) { throw finalError; }

return (finalResult === undefined) ? result : finalResult;
};
} else {
Ember.tryFinally = function(tryable, finalizer, binding) {
var result, finalResult;

binding = binding || this;

try {
result = tryable.call(binding);
} finally {
finalResult = finalizer.call(binding);
}

return (finalResult === undefined) ? result : finalResult;
};
}

/**
Provides try { } catch finally { } functionality, while working
around Safari's double finally bug.
@method tryCatchFinally
@for Ember
@param {Function} function The function to run the try callback
@param {Function} function The function to run the catchable callback
@param {Function} function The function to run the finally callback
@param [binding]
@return {anything} The return value is the that of the finalizer,
unless that value is undefined, in which case it is the return value
of the tryable.
*/
if (needsFinallyFix) {
Ember.tryCatchFinally = function(tryable, catchable, finalizer, binding) {
var result, finalResult, finalError, finalReturn;

binding = binding || this;

try {
result = tryable.call(binding);
} catch(error) {
result = catchable.call(binding, error);
} finally {
try {
finalResult = finalizer.call(binding);
} catch (e){
finalError = e;
}
}

if (finalError) { throw finalError; }

return (finalResult === undefined) ? result : finalResult;
};
} else {
Ember.tryCatchFinally = function(tryable, catchable, finalizer, binding) {
var result, finalResult;

binding = binding || this;

try {
result = tryable.call(binding);
} catch(error) {
result = catchable.call(binding, error);
} finally {
finalResult = finalizer.call(binding);
}

return (finalResult === undefined) ? result : finalResult;
};
}

0 comments on commit 917da11

Please sign in to comment.