Merge remote-tracking branch 'origin/main' into fix/bridge_memory_leaks

openkraken · Dec 2, 2021 · ff5d2d6 · ff5d2d6
2 parents ca75050 + 6ed502e
commit ff5d2d6
Show file tree

Hide file tree

Showing 16 changed files with 179 additions and 41 deletions.
diff --git a/bridge/bindings/qjs/module_manager.cc b/bridge/bindings/qjs/module_manager.cc
@@ -100,23 +100,26 @@ JSValue krakenInvokeModule(QjsContext* ctx, JSValueConst this_val, int argc, JSV
     callbackValue = argv[3];
   }
 
-  if (getDartMethod()->invokeModule == nullptr) {
-#if FLUTTER_BACKEND
-    return JS_ThrowTypeError(ctx, "Failed to execute '__kraken_invoke_module__': dart method (invokeModule) is not registered.");
-#else
-    return JS_NULL;
-#endif
-  }
-
   std::unique_ptr<NativeString> moduleName = jsValueToNativeString(ctx, moduleNameValue);
   std::unique_ptr<NativeString> method = jsValueToNativeString(ctx, methodValue);
   std::unique_ptr<NativeString> params;
   if (!JS_IsNull(paramsValue)) {
     JSValue stringifyedValue = JS_JSONStringify(ctx, paramsValue, JS_NULL, JS_NULL);
+    // JS_JSONStringify may return JS_EXCEPTION if object is not valid. Return JS_EXCEPTION and let quickjs to handle it.
+    if (JS_IsException(stringifyedValue))
+      return stringifyedValue;
     params = jsValueToNativeString(ctx, stringifyedValue);
     JS_FreeValue(ctx, stringifyedValue);
   }
 
+  if (getDartMethod()->invokeModule == nullptr) {
+#if FLUTTER_BACKEND
+    return JS_ThrowTypeError(ctx, "Failed to execute '__kraken_invoke_module__': dart method (invokeModule) is not registered.");
+#else
+    return JS_NULL;
+#endif
+  }
+
   ModuleContext* moduleContext;
   if (JS_IsNull(callbackValue)) {
     auto emptyFunction = [](QjsContext* ctx, JSValueConst this_val, int argc, JSValueConst* argv) -> JSValue { return JS_NULL; };

diff --git a/bridge/bindings/qjs/module_manager_test.cc b/bridge/bindings/qjs/module_manager_test.cc
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2021 Alibaba Inc. All rights reserved.
+ * Author: Kraken Team.
+ */
+
+#include <gtest/gtest.h>
+#include "bridge_qjs.h"
+#include "host_object.h"
+#include "js_context.h"
+
+namespace kraken::binding::qjs {
+
+TEST(ModuleManager, shouldThrowErrorWhenBadJSON) {
+  bool static errorCalled = false;
+  auto* bridge = new kraken::JSBridge(0, [](int32_t contextId, const char* errmsg) {
+    EXPECT_STREQ(errmsg,
+                 "TypeError: circular reference\n"
+                 "    at __kraken_invoke_module__ (native)\n"
+                 "    at <anonymous> (internal://:616)\n"
+                 "    at Promise (native)\n"
+                 "    at invokeMethod (internal://:617)\n"
+                 "    at <eval> (vm://:12)\n");
+    errorCalled = true;
+  });
+  kraken::JSBridge::consoleMessageHandler = [](void* ctx, const std::string& message, int logLevel) {};
+
+  auto& context = bridge->getContext();
+
+  std::string code = std::string(R"(
+let object = {
+    key: {
+        v: {
+            a: {
+                other: null
+            }
+        }
+    }
+};
+object.other = object;
+kraken.methodChannel.invokeMethod('abc', 'fn', object);
+)");
+  context->evaluateJavaScript(code.c_str(), code.size(), "vm://", 0);
+  delete bridge;
+  EXPECT_EQ(errorCalled, true);
+}
+
+}  // namespace kraken::binding::qjs
diff --git a/bridge/bindings/qjs/native_value.cc b/bridge/bindings/qjs/native_value.cc
@@ -63,6 +63,9 @@ NativeValue Native_NewInt32(int32_t value) {
 
 NativeValue Native_NewJSON(JSContext* context, JSValue& value) {
   JSValue stringifiedValue = JS_JSONStringify(context->ctx(), value, JS_UNDEFINED, JS_UNDEFINED);
+  if (JS_IsException(stringifiedValue))
+    return Native_NewNull();
+
   // NativeString owned by NativeValue will be freed by users.
   NativeString* string = jsValueToNativeString(context->ctx(), stringifiedValue).release();
   NativeValue result = (NativeValue){

diff --git a/bridge/scripts/code_generator/src/genereate_source.ts b/bridge/scripts/code_generator/src/genereate_source.ts
@@ -340,6 +340,7 @@ function generateEventInstanceConstructorCode(object: ClassObject) {
     } else if (p.kind === PropsDeclarationKind.object) {
       propApplyCode = addIndent(`JSValue v = JS_GetProperty(m_ctx, eventInit, ${p.name}Atom);
   JSValue json = JS_JSONStringify(m_ctx, v, JS_NULL, JS_NULL);
+  if (JS_IsException(json)) return json;
   nativeEvent->${p.name} = jsValueToNativeString(m_ctx, json).release();
   JS_FreeValue(m_ctx, json);
   JS_FreeValue(m_ctx, v);`, 0);

diff --git a/bridge/test/test.cmake b/bridge/test/test.cmake
@@ -31,10 +31,11 @@ elseif($ENV{KRAKEN_JS_ENGINE} MATCHES "quickjs")
     ./bindings/qjs/dom/text_node_test.cc
     ./bindings/qjs/bom/window_test.cc
     ./bindings/qjs/dom/custom_event_test.cc
+    ./bindings/qjs/module_manager_test.cc
   )
 
   ### kraken_unit_test executable
-  add_executable(kraken_unit_test ${KRAKEN_UNIT_TEST_SOURCE} ${KRAKEN_TEST_SOURCE} ${BRIDGE_SOURCE} ../bindings/qjs/html_parser.cc ../bindings/qjs/html_parser.h)
+  add_executable(kraken_unit_test ${KRAKEN_UNIT_TEST_SOURCE} ${KRAKEN_TEST_SOURCE} ${BRIDGE_SOURCE} ../bindings/qjs/html_parser.cc ../bindings/qjs/html_parser.h ../bindings/qjs/module_manager_test.cc)
   target_include_directories(kraken_unit_test PUBLIC ./third_party/googletest/googletest/include ${BRIDGE_INCLUDE})
   target_link_libraries(kraken_unit_test gtest gtest_main ${BRIDGE_LINK_LIBS})
 

diff --git a/integration_tests/snapshots/css/css-values/px.ts.e6b7b8b21.png b/integration_tests/snapshots/css/css-values/px.ts.e6b7b8b21.png
diff --git a/integration_tests/specs/css/css-values/px.ts b/integration_tests/specs/css/css-values/px.ts
@@ -46,4 +46,26 @@ describe('px', () => {
       }, 600);
     });
   });
+
+  it('negative css length value should not work', async () => {
+    const container = createElement('div', {
+      style: {
+        background: 'yellow',
+        width: '-100px',
+        height: '-100px',
+        minWidth: '-100px',
+        maxWidth: '-200px',
+        minHeight: '-100px',
+        maxHeight: '-200px',
+        padding: '-50px',
+        border: '-10px solid green',
+      }
+    }, [
+      createText('foo')
+    ]);
+
+    document.body.appendChild(container);
+
+    await snapshot();
+  });
 });
diff --git a/kraken/lib/src/css/background.dart b/kraken/lib/src/css/background.dart
@@ -405,8 +405,8 @@ class CSSBackground {
         value == FIT_HEIGTH ||
         value == SCALE_DOWN ||
         value == FILL ||
-        CSSLength.isLength(value) ||
-        CSSPercentage.isPercentage(value);
+        CSSLength.isNonNegativeLength(value) ||
+        CSSPercentage.isNonNegativePercentage(value);
   }
 
   static bool isValidBackgroundAttachmentValue(String value) {

diff --git a/kraken/lib/src/css/border.dart b/kraken/lib/src/css/border.dart
@@ -237,7 +237,7 @@ class CSSBorderSide {
   }
 
   static bool isValidBorderWidthValue(String value) {
-    return CSSLength.isLength(value) || value == THIN || value == MEDIUM || value == THICK;
+    return CSSLength.isNonNegativeLength(value) || value == THIN || value == MEDIUM || value == THICK;
   }
 
   static BorderSide none = BorderSide(color: defaultBorderColor, width: 0.0, style: BorderStyle.none);

diff --git a/kraken/lib/src/css/margin.dart b/kraken/lib/src/css/margin.dart
@@ -22,7 +22,6 @@ mixin CSSMarginMixin on RenderStyleBase {
       bottom: marginBottom.computedValue,
       top: marginTop.computedValue
     ).resolve(TextDirection.ltr);
-    assert(insets.isNonNegative);
     return insets;
   }
 

diff --git a/kraken/lib/src/css/style_declaration.dart b/kraken/lib/src/css/style_declaration.dart
@@ -263,6 +263,14 @@ class CSSStyleDeclaration {
     switch (propertyName) {
       case WIDTH:
       case HEIGHT:
+        // Validation length type
+        if (!CSSLength.isNonNegativeLength(normalizedValue) &&
+          !CSSLength.isAuto(normalizedValue) &&
+          !CSSPercentage.isNonNegativePercentage(normalizedValue)
+        ) {
+          return false;
+        }
+        break;
       case TOP:
       case LEFT:
       case RIGHT:
@@ -282,8 +290,8 @@ class CSSStyleDeclaration {
       case MAX_WIDTH:
       case MAX_HEIGHT:
         if (normalizedValue != NONE &&
-          !CSSLength.isLength(normalizedValue) &&
-          !CSSPercentage.isPercentage(normalizedValue)
+          !CSSLength.isNonNegativeLength(normalizedValue) &&
+          !CSSPercentage.isNonNegativePercentage(normalizedValue)
         ) {
           return false;
         }
@@ -294,8 +302,8 @@ class CSSStyleDeclaration {
       case PADDING_LEFT:
       case PADDING_BOTTOM:
       case PADDING_RIGHT:
-        if (!CSSLength.isLength(normalizedValue) &&
-          !CSSPercentage.isPercentage(normalizedValue)
+        if (!CSSLength.isNonNegativeLength(normalizedValue) &&
+          !CSSPercentage.isNonNegativePercentage(normalizedValue)
         ) {
           return false;
         }
@@ -304,7 +312,7 @@ class CSSStyleDeclaration {
       case BORDER_TOP_WIDTH:
       case BORDER_LEFT_WIDTH:
       case BORDER_RIGHT_WIDTH:
-        if (!CSSLength.isLength(normalizedValue)) {
+        if (!CSSLength.isNonNegativeLength(normalizedValue)) {
           return false;
         }
         break;

diff --git a/kraken/lib/src/css/style_property.dart b/kraken/lib/src/css/style_property.dart
@@ -67,7 +67,7 @@ List<String> _splitBySpace(String value) {
 
 class CSSStyleProperty {
   static void setShorthandPadding(Map<String, String?> properties, String shorthandValue) {
-    List<String?>? values = _getEdgeValues(shorthandValue);
+    List<String?>? values = _getEdgeValues(shorthandValue, isNonNegativeLengthOrPercentage: true);
     if (values == null) return;
 
     properties[PADDING_TOP] = values[0];
@@ -84,7 +84,7 @@ class CSSStyleProperty {
   }
 
   static void setShorthandMargin(Map<String, String?> properties, String shorthandValue) {
-    List<String?>? values = _getEdgeValues(shorthandValue, isLengthOrPercentage: false);
+    List<String?>? values = _getEdgeValues(shorthandValue);
     if (values == null) return;
 
     properties[MARGIN_TOP] = values[0];
@@ -289,7 +289,7 @@ class CSSStyleProperty {
         borderLeftColor = values[2];
       }
     } else if (property == BORDER_WIDTH) {
-      List<String?>? values = _getEdgeValues(shorthandValue);
+      List<String?>? values = _getEdgeValues(shorthandValue, isNonNegativeLength: true);
       if (values == null) return;
 
       borderTopWidth = values[0];
@@ -298,7 +298,7 @@ class CSSStyleProperty {
       borderLeftWidth = values[3];
     } else if (property == BORDER_STYLE) {
       // @TODO: validate value
-      List<String?>? values = _getEdgeValues(shorthandValue, isLengthOrPercentage: false);
+      List<String?>? values = _getEdgeValues(shorthandValue);
       if (values == null) return;
 
       borderTopStyle = values[0];
@@ -307,7 +307,7 @@ class CSSStyleProperty {
       borderLeftStyle = values[3];
     } else if (property == BORDER_COLOR) {
       // @TODO: validate value
-      List<String?>? values = _getEdgeValues(shorthandValue, isLengthOrPercentage: false);
+      List<String?>? values = _getEdgeValues(shorthandValue);
       if (values == null) return;
 
       borderTopColor = values[0];
@@ -429,7 +429,7 @@ class CSSStyleProperty {
 
   static List<String?>? _getBorderRaidusValues(String shorthandProperty) {
     if (!shorthandProperty.contains('/')) {
-      return _getEdgeValues(shorthandProperty);
+      return _getEdgeValues(shorthandProperty, isNonNegativeLengthOrPercentage: true);
     }
 
     List radius = shorthandProperty.split(_slashRegExp);
@@ -446,8 +446,8 @@ class CSSStyleProperty {
     String firstRadius = radius[0];
     String secondRadius = radius[1];
 
-    List<String?> firstValues = _getEdgeValues(firstRadius)!;
-    List<String?> secondValues = _getEdgeValues(secondRadius)!;
+    List<String?> firstValues = _getEdgeValues(firstRadius, isNonNegativeLengthOrPercentage: true)!;
+    List<String?> secondValues = _getEdgeValues(secondRadius, isNonNegativeLengthOrPercentage: true)!;
 
     return [
       '${firstValues[0]} ${secondValues[0]}',
@@ -550,7 +550,7 @@ class CSSStyleProperty {
         style = value;
       } else if (weight == null && CSSText.isValidFontWeightValue(value)) {
         weight = value;
-      } else if (size == null && CSSLength.isLength(value)) {
+      } else if (size == null && CSSLength.isNonNegativeLength(value)) {
         size = value;
       } else if (value == '/') {
         isSizeEndAndLineHeightStart = true;
@@ -685,7 +685,7 @@ class CSSStyleProperty {
         grow = value;
       } else if (shrink == null && CSSNumber.isNumber(value)) {
         shrink = value;
-      } else if (basis == null && ((CSSLength.isLength(value) || value == AUTO))) {
+      } else if (basis == null && ((CSSLength.isNonNegativeLength(value) || value == AUTO))) {
         basis = value;
       } else {
         return null;
@@ -718,7 +718,11 @@ class CSSStyleProperty {
     return [width, style, color];
   }
 
-  static List<String?>? _getEdgeValues(String shorthandProperty, {bool isLengthOrPercentage = true}) {
+  static List<String?>? _getEdgeValues(String shorthandProperty, {
+    bool isLengthOrPercentage = false,
+    bool isNonNegativeLengthOrPercentage = false,
+    bool isNonNegativeLength = false,
+  }) {
     var properties = shorthandProperty.split(_spaceRegExp);
 
     String? topValue;
@@ -749,6 +753,20 @@ class CSSStyleProperty {
           (!CSSLength.isLength(leftValue) && !CSSPercentage.isPercentage(leftValue))) {
         return null;
       }
+    } else if (isNonNegativeLengthOrPercentage) {
+      if ((!CSSLength.isNonNegativeLength(topValue) && !CSSPercentage.isNonNegativePercentage(topValue)) ||
+        (!CSSLength.isNonNegativeLength(rightValue) && !CSSPercentage.isNonNegativePercentage(rightValue)) ||
+        (!CSSLength.isNonNegativeLength(bottomValue) && !CSSPercentage.isNonNegativePercentage(bottomValue))||
+        (!CSSLength.isNonNegativeLength(leftValue) && !CSSPercentage.isNonNegativePercentage(leftValue))) {
+        return null;
+      }
+    } else if (isNonNegativeLength) {
+      if ((!CSSLength.isNonNegativeLength(topValue)) ||
+        (!CSSLength.isNonNegativeLength(rightValue)) ||
+        (!CSSLength.isNonNegativeLength(bottomValue))||
+        (!CSSLength.isNonNegativeLength(leftValue))) {
+        return null;
+      }
     }
 
     // Assume the properties are in the usual order top, right, bottom, left.

diff --git a/kraken/lib/src/css/text.dart b/kraken/lib/src/css/text.dart
@@ -474,7 +474,7 @@ class CSSText {
   }
 
   static bool isValidLineHeightValue(String value) {
-    return CSSLength.isLength(value) || CSSPercentage.isPercentage(value) ||
+    return CSSLength.isNonNegativeLength(value) || CSSPercentage.isNonNegativePercentage(value) ||
       value == 'normal' || double.tryParse(value) != null;
   }
 
@@ -489,7 +489,7 @@ class CSSText {
   static CSSLengthValue DEFAULT_LINE_HEIGHT = CSSLengthValue.normal;
   static CSSLengthValue? resolveLineHeight(String value, RenderStyle renderStyle, String propertyName) {
     if (value.isNotEmpty) {
-      if (CSSLength.isLength(value) || CSSPercentage.isPercentage(value)) {
+      if (CSSLength.isNonNegativeLength(value) || CSSPercentage.isNonNegativePercentage(value)) {
         CSSLengthValue lineHeight = CSSLength.parseLength(value, renderStyle, propertyName);
         // Line-height 0 and negative value is considered invalid.
         if (lineHeight.computedValue != double.infinity && lineHeight.computedValue > 0) {

diff --git a/kraken/lib/src/css/values/length.dart b/kraken/lib/src/css/values/length.dart
@@ -18,7 +18,10 @@ const _1Q = _1cm / 40; // 1Q = 1/40th of 1cm
 const _1pc = _1in / 6; // 1pc = 1/6th of 1in
 const _1pt = _1in / 72; // 1pt = 1/72th of 1in
 
-final _lengthRegExp = RegExp(r'^[+-]?(\d+)?(\.\d+)?px|rpx|vw|vh|vmin|vmax|rem|em|in|cm|mm|pc|pt$', caseSensitive: false);
+final String _unitRegStr = '(px|rpx|vw|vh|vmin|vmax|rem|em|in|cm|mm|pc|pt)';
+final _lengthRegExp = RegExp(r'^[+-]?(\d+)?(\.\d+)?' + _unitRegStr + r'$', caseSensitive: false);
+final _negativeZeroRegExp = RegExp(r'^-(0+)?(\.0+)?' + _unitRegStr + r'$', caseSensitive: false);
+final _nonNegativeLengthRegExp = RegExp(r'^[+]?(\d+)?(\.\d+)?' + _unitRegStr + r'$', caseSensitive: false);
 
 enum CSSLengthType {
   // absolute units
@@ -404,7 +407,18 @@ class CSSLength {
   }
 
   static bool isLength(String? value) {
-    return value != null && (value == ZERO || _lengthRegExp.hasMatch(value));
+    return value != null && (
+      value == ZERO
+      || _lengthRegExp.hasMatch(value)
+    );
+  }
+
+  static bool isNonNegativeLength(String? value) {
+    return value != null && (
+      value == ZERO
+      || _negativeZeroRegExp.hasMatch(value) // Negative zero is considered to be equal to zero.
+      || _nonNegativeLengthRegExp.hasMatch(value)
+    );
   }
 
   static CSSLengthValue? resolveLength(String text, RenderStyle? renderStyle, String propertyName) {

diff --git a/kraken/lib/src/css/values/percentage.dart b/kraken/lib/src/css/values/percentage.dart
@@ -8,6 +8,7 @@
 import 'package:quiver/collection.dart';
 
 final _percentageRegExp = RegExp(r'^[+-]?\d+[\.]?\d*\%$', caseSensitive: false);
+final _nonNegativePercentageRegExp = RegExp(r'^[+]?\d+[\.]?\d*\%$', caseSensitive: false);
 final LinkedLruHashMap<String, double?> _cachedParsedPercentage = LinkedLruHashMap(maximumSize: 100);
 
 class CSSPercentage {
@@ -27,4 +28,8 @@ class CSSPercentage {
   static bool isPercentage(String? percentageValue) {
     return percentageValue != null && _percentageRegExp.hasMatch(percentageValue);
   }
+
+  static bool isNonNegativePercentage(String? percentageValue) {
+    return percentageValue != null && _nonNegativePercentageRegExp.hasMatch(percentageValue);
+  }
 }