Script handling rewrite (fixes #101). Fix benchmark. Bump version.

inikulin · Jan 27, 2016 · 3c195ec · 3c195ec
1 parent c62115c
commit 3c195ec
Show file tree

Hide file tree

Showing 6 changed files with 71 additions and 49 deletions.
diff --git a/docs/07_version_history.md b/docs/07_version_history.md
@@ -1,5 +1,8 @@
 # Version history
 
+## 2.1.5
+ * Fixed: ParserStream accidentally hangs up on scripts (GH [#101](https://github.com/inikulin/parse5/issues/101)).
+
 ## 2.1.4
  * Fixed: Keep ParserStream sync for the inline scripts (GH [#98](https://github.com/inikulin/parse5/issues/98) follow up).
 

diff --git a/lib/parser/index.js b/lib/parser/index.js
@@ -354,11 +354,11 @@ function getSearchableIndexInputAttrs(isindexStartTagToken) {
 }
 
 //Parser
-var Parser = module.exports = function (options, scriptHandler) {
+var Parser = module.exports = function (options) {
     this.options = mergeOptions(DEFAULT_OPTIONS, options);
 
     this.treeAdapter = this.options.treeAdapter;
-    this.scriptHandler = scriptHandler;
+    this.pendingScript = null;
 
     if (this.options.locationInfo)
         locationInfoMixin.assign(this);
@@ -370,7 +370,7 @@ Parser.prototype.parse = function (html) {
 
     this._bootstrap(document, null);
     this.tokenizer.write(html, true);
-    this._runParsingLoop();
+    this._runParsingLoop(null, null);
 
     return document;
 };
@@ -396,7 +396,7 @@ Parser.prototype.parseFragment = function (html, fragmentContext) {
     this._resetInsertionMode();
     this._findFormInFragmentContext();
     this.tokenizer.write(html, true);
-    this._runParsingLoop();
+    this._runParsingLoop(null, null);
 
     var rootElement = this.treeAdapter.getFirstChild(documentMock),
         fragment = this.treeAdapter.createDocumentFragment();
@@ -411,7 +411,6 @@ Parser.prototype._bootstrap = function (document, fragmentContext) {
     this.tokenizer = new Tokenizer(this.options);
 
     this.stopped = false;
-    this.pausedByScript = false;
 
     this.insertionMode = INITIAL_MODE;
     this.originalInsertionMode = '';
@@ -438,13 +437,13 @@ Parser.prototype._bootstrap = function (document, fragmentContext) {
 };
 
 //Parsing loop
-Parser.prototype._runParsingLoop = function (callback) {
-    while (!this.stopped && !this.pausedByScript) {
+Parser.prototype._runParsingLoop = function (writeCallback, scriptHandler) {
+    while (!this.stopped) {
         this._setupTokenizerCDATAMode();
 
         var token = this.tokenizer.getNextToken();
 
-        if(token.type === Tokenizer.HIBERNATION_TOKEN)
+        if (token.type === Tokenizer.HIBERNATION_TOKEN)
             break;
 
         if (this.skipNextNewLine) {
@@ -463,10 +462,23 @@ Parser.prototype._runParsingLoop = function (callback) {
 
         else
             this._processToken(token);
+
+        if (scriptHandler && this.pendingScript)
+            break;
+    }
+
+    if (scriptHandler && this.pendingScript) {
+        var script = this.pendingScript;
+
+        this.pendingScript = null;
+
+        scriptHandler(script);
+
+        return;
     }
 
-    if(callback && !this.pausedByScript)
-        callback();
+    if (writeCallback)
+        writeCallback();
 };
 
 //Text parsing
@@ -2079,13 +2091,11 @@ function eofInBody(p, token) {
 //12.2.5.4.8 The "text" insertion mode
 //------------------------------------------------------------------
 function endTagInText(p, token) {
-    var closedElement = p.openElements.current;
+    if (token.tagName === $.SCRIPT)
+        p.pendingScript = p.openElements.current;
 
     p.openElements.pop();
     p.insertionMode = p.originalInsertionMode;
-
-    if (p.scriptHandler && token.tagName === $.SCRIPT)
-        p.scriptHandler(closedElement);
 }
 
 

diff --git a/lib/parser/stream.js b/lib/parser/stream.js
@@ -30,11 +30,11 @@ var WritableStream = require('stream').Writable,
 var ParserStream = module.exports = function (options) {
     WritableStream.call(this);
 
-    this.parser = new Parser(options, this.scriptHandler.bind(this));
+    this.parser = new Parser(options);
 
     this.lastChunkWritten = false;
     this.writeCallback = null;
-    this.parserLoopLock = false;
+    this.pausedByScript = false;
 
     /**
      * The resulting document node.
@@ -48,6 +48,7 @@ var ParserStream = module.exports = function (options) {
 
     this._resume = this._resume.bind(this);
     this._documentWrite = this._documentWrite.bind(this);
+    this._scriptHandler = this._scriptHandler.bind(this);
 
     this.parser._bootstrap(this.document, null);
 };
@@ -68,18 +69,11 @@ ParserStream.prototype.end = function (chunk, encoding, callback) {
 
 //Scriptable parser implementation
 ParserStream.prototype._runParsingLoop = function () {
-    // NOTE: This helps avoid reentrant invocation of parsing loop
-    // in cases then `resume` called synchronously
-    // (see: https://github.com/inikulin/parse5/issues/98).
-    if (!this.parserLoopLock) {
-        this.parserLoopLock = true;
-        this.parser._runParsingLoop(this.writeCallback);
-        this.parserLoopLock = false;
-    }
+    this.parser._runParsingLoop(this.writeCallback, this._scriptHandler);
 };
 
 ParserStream.prototype._resume = function () {
-    if (!this.parser.pausedByScript)
+    if (!this.pausedByScript)
         throw new Error('Parser was already resumed');
 
     while (this.pendingHtmlInsertions.length) {
@@ -88,7 +82,7 @@ ParserStream.prototype._resume = function () {
         this.parser.tokenizer.insertHtmlAtCurrentPos(html);
     }
 
-    this.parser.pausedByScript = false;
+    this.pausedByScript = false;
 
     //NOTE: keep parsing if we don't wait for the next input chunk
     if (this.parser.tokenizer.active)
@@ -100,9 +94,9 @@ ParserStream.prototype._documentWrite = function (html) {
         this.pendingHtmlInsertions.push(html);
 };
 
-ParserStream.prototype.scriptHandler = function (scriptElement) {
+ParserStream.prototype._scriptHandler = function (scriptElement) {
     if (this.listeners('script').length) {
-        this.parser.pausedByScript = true;
+        this.pausedByScript = true;
 
         /**
          * Raised then parser encounters a `<script>` element.
@@ -140,5 +134,7 @@ ParserStream.prototype.scriptHandler = function (scriptElement) {
 
         this.emit('script', scriptElement, this._documentWrite, this._resume);
     }
+    else
+        this._runParsingLoop();
 };
 
diff --git a/package.json b/package.json
@@ -1,7 +1,7 @@
 {
   "name": "parse5",
   "description": "WHATWG HTML5 specification-compliant, fast and ready for production HTML parsing/serialization toolset for Node.js",
-  "version": "2.1.4",
+  "version": "2.1.5",
   "author": "Ivan Nikulin <ifaaan@gmail.com> (https://github.com/inikulin)",
   "contributors": [
     "Alan Clarke (https://github.com/alanclarke)",

diff --git a/test/benchmark/bench-micro.js b/test/benchmark/bench-micro.js
@@ -21,15 +21,10 @@ global.micro = testUtils
         };
     });
 
-global.runMicro = function (parser, newAPI) {
+global.runMicro = function (parser) {
     for (var i = 0; i < micro.length; i++) {
-        if (micro[i].fragmentContext) {
-            if (newAPI)
-                parser.parseFragment(micro[i].fragmentContext, micro[i].html);
-            else
-                parser.parseFragment(micro[i].html, micro[i].fragmentContext);
-
-        }
+        if (micro[i].fragmentContext)
+            parser.parseFragment(micro[i].fragmentContext, micro[i].html);
         else
             parser.parse(micro[i].html);
     }
@@ -42,7 +37,7 @@ module.exports = {
             name: 'Working copy',
 
             fn: function () {
-                runMicro(workingCopy, true);
+                runMicro(workingCopy);
             }
         },
         {

diff --git a/test/fixtures/scripting_test.js b/test/fixtures/scripting_test.js
@@ -81,18 +81,36 @@ testUtils.generateTestsForEachTreeAdapter(module.exports, function (_test, treeA
                     .catch(done);
             };
         });
-});
 
-exports['Regression - Synchronously calling resume() leads to crash (GH-98)'] = function (done) {
-    var parser = new ParserStream({
-        locationInfo: true
-    });
+    _test['Regression - Synchronously calling resume() leads to crash (GH-98)'] = function (done) {
+        var parser = new ParserStream({treeAdapter: treeAdapter});
 
-    parser.on('script', function (el, docWrite, resume) {
-        resume();
-    });
+        parser.on('script', function (el, docWrite, resume) {
+            resume();
+        });
+
+        parser.end('<!doctype html><script>abc</script>');
+
+        process.nextTick(done);
+    };
+
+
+    _test['Regression - Parsing loop lock causes accidental hang ups (GH-101)'] = function (done) {
+        var parser = new ParserStream({treeAdapter: treeAdapter});
+
+        parser.once('finish', function () {
+            done();
+        });
+
+        parser.on('script', function (scriptElement, documentWrite, resume) {
+            process.nextTick(function () {
+                resume();
+            });
+        });
+
+        parser.write('<script>yo</script>');
+        parser.end('dawg');
+    };
+});
 
-    parser.end('<!doctype html><script>abc</script>');
 
-    process.nextTick(done);
-};