fix: Avoid timing issue with axe cleanup method

dequelabs · Mar 19, 2018 · 24ea6a7 · 24ea6a7
1 parent dc0241b
commit 24ea6a7
Show file tree

Hide file tree

Showing 13 changed files with 174 additions and 73 deletions.
diff --git a/lib/core/public/load.js b/lib/core/public/load.js
@@ -18,7 +18,11 @@ function runCommand(data, keepalive, callback) {
 
 	switch (data.command) {
 		case 'rules':
-			return runRules(context, options, resolve, reject);
+			return runRules(context, options, function (results, cleanup) {
+				resolve(results);
+				// Cleanup AFTER resolve so that selectors can be generated
+				cleanup();
+			}, reject);
 		case 'cleanup-plugin':
 			return cleanupPlugins(resolve, reject);
 		default:

diff --git a/lib/core/public/run-rules.js b/lib/core/public/run-rules.js
@@ -1,9 +1,8 @@
 /*global Context */
 /*exported runRules */
 
-function complete (cb, ...args) {
-	cb(...args);
-	// Clean up after resolve / reject
+// Clean up after resolve / reject
+function cleanup () {
 	axe._tree = undefined;
 	axe._selectorData = undefined;
 }
@@ -13,7 +12,8 @@ function complete (cb, ...args) {
  * @private
  * @param  {Object}   context  The `Context` specification object @see Context
  * @param  {Array}    options  Optional RuleOptions
- * @param  {Function} callback The function to invoke when analysis is complete; receives an array of `RuleResult`s
+ * @param  {Function} resolve  Called when done running rules, receives ([results : Object], cleanup : Function)
+ * @param  {Function} reject   Called when execution failed, receives (err : Error)
  */
 function runRules(context, options, resolve, reject) {
 	'use strict';
@@ -22,7 +22,8 @@ function runRules(context, options, resolve, reject) {
 		axe._tree = context.flatTree;
 		axe._selectorData = axe.utils.getSelectorData(context.flatTree);
 	} catch (e) {
-		return complete(reject, e);
+		cleanup();
+		return reject(e);
 	}
 
 	var q = axe.utils.queue();
@@ -66,15 +67,18 @@ function runRules(context, options, resolve, reject) {
 				results = results.map(axe.utils.finalizeRuleResult);
 			}
 			try {
-				complete(resolve, results);
+				resolve(results, cleanup);
 			} catch(e) {
-				complete(axe.log, e);
+				cleanup();
+				axe.log(e);
 			}
 		} catch (e) {
-			complete(reject, e);
+			cleanup();
+			reject(e);
 		}
 	}).catch((e) => {
-		complete(reject, e);
+		cleanup();
+		reject(e);
 	});
 }
 

diff --git a/lib/core/public/run.js b/lib/core/public/run.js
@@ -112,33 +112,35 @@ axe.run = function (context, options, callback) {
 		});
 	}
 
-	axe._runRules(context, options, function (rawResults) {
-		let respond = function (results) {
-			try {
-				callback(null, results);
-			} catch(e) {
-				axe.log(e);
-			}
-			resolve(results);
-		};
-		if (options.performanceTimer) {
-			axe.utils.performanceTimer.end();
-		}
-
-		try {
-			let reporter = axe.getReporter(options.reporter);
-			let results = reporter(rawResults, options, respond);
-			if (results !== undefined) {
-				respond(results);
-			}
-		} catch (err) {
-			callback(err);
-			reject(err);
-		}
-	}, function (err) {
-		callback(err);
-		reject(err);
-	});
-
-	return p;
-};
+	axe._runRules(context, options, function (rawResults, cleanup) {
+ 		let respond = function (results) {
+			cleanup();
+ 			try {
+ 				callback(null, results);
+ 			} catch(e) {
+ 				axe.log(e);
+ 			}
+ 			resolve(results);
+ 		};
+ 		if (options.performanceTimer) {
+ 			axe.utils.performanceTimer.end();
+ 		}
+
+ 		try {
+ 			let reporter = axe.getReporter(options.reporter);
+ 			let results = reporter(rawResults, options, respond);
+ 			if (results !== undefined) {
+ 				respond(results);
+ 			}
+ 		} catch (err) {
+			cleanup();
+ 			callback(err);
+ 			reject(err);
+ 		}
+ 	}, function (err) {
+ 		callback(err);
+ 		reject(err);
+ 	});
+
+ 	return p;
+};
diff --git a/test/core/public/run-rules.js b/test/core/public/run-rules.js
@@ -651,7 +651,7 @@ describe('runRules', function () {
 		}, isNotCalled);
 	});
 
-	it('should clear up axe._tree / axe._selectorData after resolving', function (done) {
+	it('should return a cleanup method', function (done) {
 		axe._load({ rules: [{
 			id: 'html',
 			selector: 'html',
@@ -663,14 +663,14 @@ describe('runRules', function () {
 			}
 		}], messages: {}});
 
-		runRules(document, {}, function resolve() {
+		runRules(document, {}, function resolve(out, cleanup) {
 			assert.isDefined(axe._tree);
 			assert.isDefined(axe._selectorData);
-			setTimeout(function () {
-				assert.isUndefined(axe._tree);
-				assert.isUndefined(axe._selectorData);
-				done();
-			}, 10);
+
+			cleanup();
+			assert.isUndefined(axe._tree);
+			assert.isUndefined(axe._selectorData);
+			done();
 		}, isNotCalled);
 	});
 
@@ -681,18 +681,10 @@ describe('runRules', function () {
 
 		createFrames(function () {
 			setTimeout(function () {
-				runRules(document, {}, function () {
-					assert.ok(false, 'You shall not pass!');
+				runRules(document, {}, isNotCalled, function () {
+					assert.isUndefined(axe._tree);
+					assert.isUndefined(axe._selectorData);
 					done();
-				},
-				function () {
-					assert.isDefined(axe._tree);
-					assert.isDefined(axe._selectorData);
-					setTimeout(function () {
-						assert.isUndefined(axe._tree);
-						assert.isUndefined(axe._selectorData);
-						done();
-					}, 10);
 				});
 			}, 100);
 		});

diff --git a/test/core/public/run.js b/test/core/public/run.js
@@ -122,7 +122,7 @@ describe('axe.run', function () {
 		it('gives results to the second argument on the callback', function (done) {
 			axe._runRules = function (ctxt, opt, resolve) {
 				axe._runRules = origRunRules;
-				resolve('MB Bomb');
+				resolve('MB Bomb', noop);
 			};
 
 			axe.run({ reporter: 'raw' }, function (err, result) {
@@ -135,7 +135,7 @@ describe('axe.run', function () {
 		it('does not run the callback twice if it throws', function (done) {
 			var calls = 0;
 			axe._runRules = function (ctxt, opt, resolve) {
-				resolve([]);
+				resolve([], noop);
 			};
 
 			var log = axe.log;
@@ -155,13 +155,30 @@ describe('axe.run', function () {
 				throw new Error('err');
 			});
 		});
+
+		it('is called after cleanup', function (done) {
+			var isClean = false;
+			axe._runRules = function (ctxt, opt, resolve) {
+				axe._runRules = origRunRules;
+				// Check that cleanup is called before the callback is executed
+				resolve('MB Bomb', function cleanup() {
+					isClean = true;
+				});
+			};
+
+			axe.run({ reporter: 'raw' }, function () {
+				assert.isTrue(isClean, 'cleanup must be called first');
+				done();
+			});
+		});
 	});
 
 
 	describe('promise result', function () {
 		/*eslint indent: 0*/
-		it('returns an error to catch if axe fails',
-		(!window.Promise) ? undefined :  function (done) {
+		var promiseIt = window.Promise ? it : it.skip;
+
+		promiseIt('returns an error to catch if axe fails', function (done) {
 			axe._runRules = function (ctxt, opt, resolve, reject) {
 				axe._runRules = origRunRules;
 				reject('I surrender!');
@@ -177,11 +194,10 @@ describe('axe.run', function () {
 			assert.instanceOf(p, window.Promise);
 		});
 
-		it('returns a promise if no callback was given',
-		(!window.Promise) ? undefined :  function (done) {
+		promiseIt('returns a promise if no callback was given', function (done) {
 			axe._runRules = function (ctxt, opt, resolve) {
 				axe._runRules = origRunRules;
-				resolve('World party');
+				resolve('World party', noop);
 			};
 
 			var p = axe.run({ reporter: 'raw' });
@@ -193,10 +209,9 @@ describe('axe.run', function () {
 			assert.instanceOf(p, window.Promise);
 		});
 
-		it('does not error if then() throws',
-		(!window.Promise) ? undefined :  function (done) {
+		promiseIt('does not error if then() throws', function (done) {
 			axe._runRules = function (ctxt, opt, resolve) {
-				resolve([]);
+				resolve([], noop);
 			};
 
 			axe.run()
@@ -211,6 +226,24 @@ describe('axe.run', function () {
 				done();
 			});
 		});
+
+		promiseIt('is called after cleanup', function (done) {
+			var isClean = false;
+			axe._runRules = function (ctxt, opt, resolve) {
+				axe._runRules = origRunRules;
+				// Check that cleanup is called before the callback is executed
+				resolve('MB Bomb', function cleanup () {
+					isClean = true;
+				});
+			};
+
+			axe.run({ reporter: 'raw' })
+			.then(function () {
+				assert(isClean, 'cleanup must be called first');
+				done();
+			})
+			.catch(done);
+		});
 	});
 
 

diff --git a/test/core/reporters/na.js b/test/core/reporters/na.js
@@ -84,7 +84,7 @@ describe('reporters - na', function() {
 		});
 		orig = axe._runRules;
 		axe._runRules = function(ctxt, options, cb) {
-			cb(results);
+			cb(results, function noop () {});
 		};
 	});
 

diff --git a/test/core/reporters/no-passes.js b/test/core/reporters/no-passes.js
@@ -74,7 +74,7 @@ describe('reporters - no-passes', function() {
 		});
 		orig = axe._runRules;
 		axe._runRules = function(ctxt, options, cb) {
-			cb(results);
+			cb(results, function noop () {});
 		};
 	});
 

diff --git a/test/core/reporters/raw.js b/test/core/reporters/raw.js
@@ -5,7 +5,7 @@ describe('reporters - raw', function () {
 		axe._load({});
 		var orig = axe._runRules;
 		axe._runRules = function (_, __, cb) {
-			cb('foo');
+			cb('foo', function noop () {});
 		};
 
 		axe.run({ reporter: 'raw'}, function (err, results) {

diff --git a/test/core/reporters/v1.js b/test/core/reporters/v1.js
@@ -132,7 +132,7 @@ describe('reporters - v1', function() {
 		});
 		orig = axe._runRules;
 		axe._runRules = function(ctxt, options, cb) {
-			cb(results);
+			cb(results, function noop () {});
 		};
 	});
 

diff --git a/test/core/reporters/v2.js b/test/core/reporters/v2.js
@@ -74,7 +74,7 @@ describe('reporters - v2', function() {
 		});
 		orig = axe._runRules;
 		axe._runRules = function(ctxt, options, cb) {
-			cb(results);
+			cb(results, function noop () {});
 		};
 	});
 

diff --git a/test/integration/full/rerun/frames/frame.html b/test/integration/full/rerun/frames/frame.html
@@ -0,0 +1,10 @@
+<!doctype html>
+<html lang="en">
+<head>
+  <meta charset="utf8">
+  <script src="/axe.js"></script>
+</head>
+<body>
+  <h1>Just a frame</h1>
+</body>
+</html>
diff --git a/test/integration/full/rerun/rerun.html b/test/integration/full/rerun/rerun.html
@@ -0,0 +1,26 @@
+<!doctype html>
+<html lang="en">
+<head>
+<title>Rerun Test</title>
+
+<link rel="stylesheet" type="text/css" href="/node_modules/mocha/mocha.css" />
+<script src="/node_modules/mocha/mocha.js"></script>
+<script src="/node_modules/chai/chai.js"></script>
+<script src="/axe.js"></script>
+<script src="/test/testutils.js"></script>
+<script>
+	mocha.setup({
+		timeout: 10000,
+		ui: 'bdd'
+	});
+	var assert = chai.assert;
+</script>
+<script src="/test/integration/no-ui-reporter.js"></script>
+</head>
+<body>
+	<iframe src="frames/frame.html"></iframe>
+	<div id="mocha"></div>
+	<script src="rerun.js"></script>
+	<script src="/test/integration/adapter.js"></script>
+</body>
+</html>