Bug 1998923 - Make better inline and never-inline decisions around EnsureBufferSpace in the tokenizer. r=farre

Differential Revision: https://phabricator.services.mozilla.com/D271752

parser/html/javasrc/Portability.java

@@ -31,6 +31,7 @@ import nu.validator.htmlparser.common.Interner;
 
 public final class Portability {
 
+    // [NOCPP[
     public static int checkedAdd(int a, int b) throws SAXException {
         // This can't be translated code, because in C++ signed integer overflow is UB, so the below code would be wrong.
         assert a >= 0;
@@ -41,6 +42,7 @@ public final class Portability {
         }
         return sum;
     }
+    // ]NOCPP]
 
     // Allocating methods
 

parser/html/javasrc/Tokenizer.java

@@ -982,12 +982,9 @@ public class Tokenizer implements Locator, Locator2 {
      * @param c
      *            the UTF-16 code unit to append
      */
-    private void appendStrBuf(char c) {
-        // CPPONLY: assert strBufLen < strBuf.length: "Previous buffer length insufficient.";
+    @Inline private void appendStrBuf(char c) {
         // CPPONLY: if (strBufLen == strBuf.length) {
-        // CPPONLY:     if (!EnsureBufferSpace(1)) {
-        // CPPONLY:         assert false: "RELEASE: Unable to recover from buffer reallocation failure";
-        // CPPONLY:     } // TODO: Add telemetry when outer if fires but inner does not
+        // CPPONLY:     EnsureBufferSpaceShouldNeverHappen(1);
         // CPPONLY: }
         strBuf[strBufLen++] = c;
     }
@@ -1094,13 +1091,12 @@ public class Tokenizer implements Locator, Locator2 {
         // ]NOCPP]
     }
 
-    private void appendStrBuf(@NoLength char[] buffer, int offset, int length) throws SAXException {
-        int newLen = Portability.checkedAdd(strBufLen, length);
-        // CPPONLY: assert newLen <= strBuf.length: "Previous buffer length insufficient.";
+    @Inline private void appendStrBuf(@NoLength char[] buffer, int offset, int length) throws SAXException {
+        // Years of crash stats have shown that the this addition doesn't overflow, as it logically
+        // shouldn't.
+        int newLen = strBufLen + length;
         // CPPONLY: if (strBuf.length < newLen) {
-        // CPPONLY:     if (!EnsureBufferSpace(length)) {
-        // CPPONLY:         assert false: "RELEASE: Unable to recover from buffer reallocation failure";
-        // CPPONLY:     } // TODO: Add telemetry when outer if fires but inner does not
+        // CPPONLY:     EnsureBufferSpaceShouldNeverHappen(length);
         // CPPONLY: }
         System.arraycopy(buffer, offset, strBuf, strBufLen, length);
         strBufLen = newLen;

parser/html/nsHtml5Portability.cpp

@@ -9,14 +9,6 @@
 #include "nsString.h"
 #include "mozilla/CheckedInt.h"
 
-int32_t nsHtml5Portability::checkedAdd(int32_t a, int32_t b) {
-  mozilla::CheckedInt<int32_t> sum(a);
-  sum += b;
-  MOZ_RELEASE_ASSERT(sum.isValid(),
-                     "HTML input too large for signed 32-bit integer.");
-  return sum.value();
-}
-
 nsAtom* nsHtml5Portability::newLocalNameFromBuffer(char16_t* buf,
                                                    int32_t length,
                                                    nsHtml5AtomTable* interner) {

parser/html/nsHtml5Portability.h

@@ -54,7 +54,6 @@ class nsHtml5StateSnapshot;
 
 class nsHtml5Portability {
  public:
-  static int32_t checkedAdd(int32_t a, int32_t b);
   static nsAtom* newLocalNameFromBuffer(char16_t* buf, int32_t length,
                                         nsHtml5AtomTable* interner);
   static nsHtml5String newStringFromBuffer(char16_t* buf, int32_t offset,

parser/html/nsHtml5Tokenizer.cpp

@@ -241,29 +241,6 @@ void nsHtml5Tokenizer::emitOrAppendCharRefBuf(int32_t returnState) {
   }
 }
 
-void nsHtml5Tokenizer::appendStrBuf(char16_t c) {
-  MOZ_ASSERT(strBufLen < strBuf.length, "Previous buffer length insufficient.");
-  if (MOZ_UNLIKELY(strBufLen == strBuf.length)) {
-    if (MOZ_UNLIKELY(!EnsureBufferSpace(1))) {
-      MOZ_CRASH("Unable to recover from buffer reallocation failure");
-    }
-  }
-  strBuf[strBufLen++] = c;
-}
-
-void nsHtml5Tokenizer::appendStrBuf(char16_t* buffer, int32_t offset,
-                                    int32_t length) {
-  int32_t newLen = nsHtml5Portability::checkedAdd(strBufLen, length);
-  MOZ_ASSERT(newLen <= strBuf.length, "Previous buffer length insufficient.");
-  if (MOZ_UNLIKELY(strBuf.length < newLen)) {
-    if (MOZ_UNLIKELY(!EnsureBufferSpace(length))) {
-      MOZ_CRASH("Unable to recover from buffer reallocation failure");
-    }
-  }
-  nsHtml5ArrayCopy::arraycopy(buffer, offset, strBuf, strBufLen, length);
-  strBufLen = newLen;
-}
-
 void nsHtml5Tokenizer::emitComment(int32_t provisionalHyphens, int32_t pos) {
   RememberGt(pos);
   tokenHandler->comment(strBuf, 0, strBufLen - provisionalHyphens);

parser/html/nsHtml5Tokenizer.h

@@ -359,7 +359,12 @@ class nsHtml5Tokenizer {
     strBufLen = 0;
   }
 
-  void appendStrBuf(char16_t c);
+  inline void appendStrBuf(char16_t c) {
+    if (MOZ_UNLIKELY(strBufLen == strBuf.length)) {
+      EnsureBufferSpaceShouldNeverHappen(1);
+    }
+    strBuf[strBufLen++] = c;
+  }
 
  protected:
   inline nsHtml5String strBufToString() {
@@ -392,7 +397,15 @@ class nsHtml5Tokenizer {
     appendStrBuf(c);
   }
 
-  void appendStrBuf(char16_t* buffer, int32_t offset, int32_t length);
+  inline void appendStrBuf(char16_t* buffer, int32_t offset, int32_t length) {
+    int32_t newLen = strBufLen + length;
+    if (MOZ_UNLIKELY(strBuf.length < newLen)) {
+      EnsureBufferSpaceShouldNeverHappen(length);
+    }
+    nsHtml5ArrayCopy::arraycopy(buffer, offset, strBuf, strBufLen, length);
+    strBufLen = newLen;
+  }
+
   inline void appendCharRefBufToStrBuf() {
     appendStrBuf(charRefBuf, 0, charRefBufLen);
     charRefBufLen = 0;

parser/html/nsHtml5TokenizerCppSupplement.h

@@ -61,6 +61,18 @@ bool nsHtml5Tokenizer::EnsureBufferSpace(int32_t aLength) {
   return true;
 }
 
+MOZ_COLD MOZ_NEVER_INLINE void
+nsHtml5Tokenizer::EnsureBufferSpaceShouldNeverHappen(int32_t aLength) {
+  MOZ_DIAGNOSTIC_ASSERT(false,
+                        "This is never supposed to happen. Please file a bug "
+                        "with steps to reproduce!");
+  if (!EnsureBufferSpace(aLength)) {
+    MOZ_CRASH(
+        "Unrecovable allocation failure in situation that should never happen. "
+        "Please file a bug with steps to reproduce!");
+  }
+}
+
 bool nsHtml5Tokenizer::TemplatePushedOrHeadPopped() {
   if (encodingDeclarationHandler) {
     return encodingDeclarationHandler->TemplatePushedOrHeadPopped();

parser/html/nsHtml5TokenizerHSupplement.h

@@ -67,6 +67,9 @@ inline nsHtml5HtmlAttributes* GetAttributes() { return attributes; }
  */
 bool EnsureBufferSpace(int32_t aLength);
 
+MOZ_COLD MOZ_NEVER_INLINE void EnsureBufferSpaceShouldNeverHappen(
+    int32_t aLength);
+
 bool TemplatePushedOrHeadPopped();
 
 void RememberGt(int32_t aPos);