From 1678d988aeaac86aa0038b4736a74c33b83c2291 Mon Sep 17 00:00:00 2001
From: Wyatt Allen <wyatta@google.com>
Date: Fri, 1 Jul 2016 11:32:25 -0700
Subject: [PATCH] Addresses a gr-diff-highlight edge case

It was possible to cause a JS error when creating a ranged comment that
started at the very end of the first line (selecting no content on that
line). The relevant null-guard needed an additional set of parens to
avoid evaluating the second OR operand with a bad argument in this case.

Addresses the null-guard boolean expressions in `_normalizeStart` and
`_normalizeEnd` and reduces the number of calls to  `_getLength` from
thrice to once per iteration. Adds a relevant unit test.

Change-Id: I98848f9f6089fd3240bda175765770c9f9c5ba30
---
 .../gr-diff-highlight/gr-diff-highlight.js    | 18 +++++-----
 .../gr-diff-highlight_test.html               | 34 +++++++++++++++++++
 .../gr-js-api-interface_test.html             |  2 +-
 3 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
index 1c3572da1a..518c7fed82 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
@@ -494,12 +494,13 @@
       }
 
       // Skip nodes before startOffset.
-      while (startNode &&
-          this._getLength(startNode) <= startOffset ||
-          this._getLength(startNode) === 0) {
-        startOffset -= this._getLength(startNode);
+      var nodeLength = this._getLength(startNode);
+      while (startNode && (nodeLength <= startOffset || nodeLength === 0)) {
+        startOffset -= nodeLength;
         startNode = startNode.nextSibling;
+        nodeLength = startNode && this._getLength(startNode);
       }
+      if (!startNode) { return null; }
 
       // Split Text node.
       if (startNode instanceof Text) {
@@ -546,12 +547,13 @@
       }
 
       // Find the node where endOffset points at.
-      while (endNode &&
-          this._getLength(endNode) < endOffset ||
-          this._getLength(endNode) === 0) {
-        endOffset -= this._getLength(endNode);
+      var nodeLength = this._getLength(endNode);
+      while (endNode && (nodeLength < endOffset || nodeLength === 0)) {
+        endOffset -= nodeLength;
         endNode = endNode.nextSibling;
+        nodeLength = endNode && this._getLength(endNode);
       }
+      if (!endNode) { return null; }
 
       if (endNode instanceof Text) {
         endNode =
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
index 0df6a20b89..8771cc14b9 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
@@ -371,6 +371,40 @@ limitations under the License.
       assert.equal(tabs[1].nextSibling.textContent, 'verbis, ');
     });
 
+    test('multiline highlight w/ start at end of 1st line', function() {
+      var diff = element.querySelector('#diffTable');
+      var startContent =
+          diff.querySelector('.left.lineNum[data-value="138"] ~ .content');
+      var betweenContent =
+          diff.querySelector('.left.lineNum[data-value="140"] ~ .content');
+      var endContent =
+          diff.querySelector('.left.lineNum[data-value="141"] ~ .content');
+      var commentThread =
+          diff.querySelector('gr-diff-comment-thread');
+      var builder = {
+        getCommentThreadByContentEl: sandbox.stub().returns(commentThread),
+        getContentByLine: sandbox.stub().returns({}),
+        getContentsByLineRange: sandbox.stub().returns([betweenContent]),
+        getLineElByChild: sandbox.stub().returns(
+            {getAttribute: sandbox.stub()}),
+      };
+      element._cachedDiffBuilder = builder;
+      element.enabled = true;
+      builder.getContentByLine.withArgs(138, 'left').returns(
+          startContent);
+      builder.getContentByLine.withArgs(141, 'left').returns(
+          endContent);
+
+      var expectedStartContentNodes = startContent.childNodes.length;
+
+      // The following should not cause an error.
+      element._applyRangedHighlight(
+          'some', 138, startContent.textContent.length, 141, 28, 'left');
+
+      assert.equal(startContent.childNodes.length, expectedStartContentNodes,
+          'Should not add a highlight to the start content');
+    });
+
     suite('single line ranges', function() {
       var diff;
       var content;
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.html b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.html
index 5936bee9c3..c12d653071 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.html
@@ -42,7 +42,7 @@ limitations under the License.
         getAccount: function() {
           return Promise.resolve({name: 'Judy Hopps'});
         },
-      })
+      });
       element = fixture('basic');
       errorStub = sinon.stub(console, 'error');
       Gerrit.install(function(p) { plugin = p; }, '0.1',