Attachment 349490 Details for Bug 180664 – [PATCH] Proposed Fix

[patch] [PATCH] Proposed Fix

regex-4.patch (text/plain), 10.49 KB, created by Joseph Pecoraro on 2018-09-11 16:41:47 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Joseph Pecoraro

Created: 2018-09-11 16:41:47 PDT

Size: 10.49 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 77060e2d2cf..f16c4ea6274 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,18 @@
>+2018-09-11  Joseph Pecoraro  <pecoraro@apple.com>
>+
>+        Web Inspector: fix test case failures in js-isLikelyStackTrace.html
>+        https://bugs.webkit.org/show_bug.cgi?id=180664
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * inspector/console/js-isLikelyStackTrace-expected.txt:
>+        * inspector/console/js-isLikelyStackTrace.html:
>+        Avoid Inspector Internal InjectedScript code in backtraces by producing
>+        the exception stacks in the page itself without going through inspector
>+        test evaluation code. This produces exception stacks more like a page.
>+        Also add some explicit tests for strings that we'd expect to be classified
>+        as exception stacks.
>+
> 2018-09-11  Woodrow Wang  <woodrow_wang@apple.com>
> 
>         Add Web API Statistics Collection
>diff --git a/LayoutTests/inspector/console/js-isLikelyStackTrace-expected.txt b/LayoutTests/inspector/console/js-isLikelyStackTrace-expected.txt
>index 9687076e026..f22cf149197 100644
>--- a/LayoutTests/inspector/console/js-isLikelyStackTrace-expected.txt
>+++ b/LayoutTests/inspector/console/js-isLikelyStackTrace-expected.txt
>@@ -5,20 +5,21 @@ Test stack trace detection heuristic.
> -- Running test case: notStacktrace
> PASS: Should NOT be a stacktrace.
> 
>--- Running test case: typeErrorWrap
>-FAIL: Should be a stacktrace.
>-    Expected: truthy
>-    Actual: false
>+-- Running test case: WI.StackTrace.isLikelyStackTrace.TypeError
>+PASS: Should be a stacktrace.
>+
>+-- Running test case: WI.StackTrace.isLikelyStackTrace.AnonymousFunctionError
>+PASS: Should be a stacktrace.
> 
>--- Running test case: getAnonymousFunctionError
>+-- Running test case: WI.StackTrace.isLikelyStackTrace.NativeFunctionCallsError
> PASS: Should be a stacktrace.
> 
>--- Running test case: testWithNativeCallInBetween
>-FAIL: Should be a stacktrace.
>-    Expected: truthy
>-    Actual: false
>+-- Running test case: WI.StackTrace.isLikelyStackTrace.ValidStackStrings
>+PASS: "a@file:///ex.html:10:6\nb@file:///ex.html:7:6\nglobal code@file:///ex.html:12:2" should be a stacktrace.
>+PASS: "global code@file:///ex.html:12:2\na@file:///ex.html:10:6\nb@file:///ex.html:7:6" should be a stacktrace.
>+PASS: "global code@file:///ex.html:1:1\nmap@[native code]" should be a stacktrace.
> 
>--- Running test case: testFormattedStrings
>+-- Running test case: WI.StackTrace.isLikelyStackTrace.InvalidStackStrings
> PASS: "video:1:2" should not be a stacktrace.
> PASS: "video/mp4:1:2" should not be a stacktrace.
> PASS: "video/mp4 : 11:22:33-44:55:66" should not be a stacktrace.
>diff --git a/LayoutTests/inspector/console/js-isLikelyStackTrace.html b/LayoutTests/inspector/console/js-isLikelyStackTrace.html
>index c81e9a65a34..85b9029fe0a 100644
>--- a/LayoutTests/inspector/console/js-isLikelyStackTrace.html
>+++ b/LayoutTests/inspector/console/js-isLikelyStackTrace.html
>@@ -2,43 +2,33 @@
> <head>
> <script src="../../http/tests/inspector/resources/inspector-test.js"></script>
> <script>
>-function typeErrorWrap()
>+function generateTypeError()
> {
>-    return typeError();
>-}
>-
>-function typeError()
>-{
>-    var object = {};
>     try {
>-        object.propertyDoesnt.exist;
>+        ({}).propertyDoesnt.exist;
>     } catch(e) {
>         console.trace();
>         return e.stack;
>     }
> }
> 
>-function testWithNativeCallInBetween()
>+function generateErrorNativeCallInBetween()
> {
>-    return [42].map(typeError)[0];
>+    return [42].map(generateTypeError)[0];
> }
> 
>-var _anonymousFunctionError = null;
>-
>+var _globalTypeError = generateTypeError();
>+var _globalErrorWithNativeCalls = generateErrorNativeCallInBetween();
>+var _globalAnonymousFunctionError = null;
> (function() {
>     var object = {};
>     try {
>         object.methodDoesntExist();
>     } catch(e) {
>-        _anonymousFunctionError = e.stack;
>+        _globalAnonymousFunctionError = e.stack;
>     }
> })();
> 
>-function getAnonymousFunctionError()
>-{
>-    return _anonymousFunctionError;
>-}
>-
> function test()
> {
>     let suite = InspectorTest.createAsyncSuite("WI.StackTrace.isLikelyStackTrace");
>@@ -53,51 +43,46 @@ function test()
>     });
> 
>     suite.addTestCase({
>-        name: "typeErrorWrap",
>-        test(resolve, reject) {
>-            InspectorTest.evaluateInPage("typeErrorWrap()", function(error, result) {
>-                if (error)
>-                    reject(error);
>-
>-                InspectorTest.expectThat(WI.StackTrace.isLikelyStackTrace(result), "Should be a stacktrace.");
>-
>-                resolve();
>-            });
>+        name: "WI.StackTrace.isLikelyStackTrace.TypeError",
>+        async test() {
>+            let result = await InspectorTest.evaluateInPage(`_globalTypeError`);
>+            InspectorTest.expectThat(WI.StackTrace.isLikelyStackTrace(result), "Should be a stacktrace.");
>         }
>     });
> 
>     suite.addTestCase({
>-        name: "getAnonymousFunctionError",
>-        test(resolve, reject) {
>-            InspectorTest.evaluateInPage("getAnonymousFunctionError()", function(error, result) {
>-                if (error)
>-                    reject(error);
>-
>-                // FIXME: this test case fails. <https://bugs.webkit.org/show_bug.cgi?id=180664>
>-                InspectorTest.expectThat(WI.StackTrace.isLikelyStackTrace(result), "Should be a stacktrace.");
>-
>-                resolve();
>-            });
>+        name: "WI.StackTrace.isLikelyStackTrace.AnonymousFunctionError",
>+        async test() {
>+            let result = await InspectorTest.evaluateInPage(`_globalAnonymousFunctionError`);
>+            InspectorTest.expectThat(WI.StackTrace.isLikelyStackTrace(result), "Should be a stacktrace.");
>         }
>     });
> 
>     suite.addTestCase({
>-        name: "testWithNativeCallInBetween",
>-        test(resolve, reject) {
>-            InspectorTest.evaluateInPage("testWithNativeCallInBetween()", function(error, result) {
>-                if (error)
>-                    reject(error);
>-
>-                InspectorTest.expectThat(WI.StackTrace.isLikelyStackTrace(result), "Should be a stacktrace.");
>+        name: "WI.StackTrace.isLikelyStackTrace.NativeFunctionCallsError",
>+        async test() {
>+            let result = await InspectorTest.evaluateInPage(`_globalErrorWithNativeCalls`);
>+            InspectorTest.expectThat(WI.StackTrace.isLikelyStackTrace(result), "Should be a stacktrace.");
>+        }
>+    });
> 
>-                resolve();
>-            });
>+    suite.addTestCase({
>+        name: "WI.StackTrace.isLikelyStackTrace.ValidStackStrings",
>+        async test() {
>+            const strings = [
>+                "a@file:///ex.html:10:6\nb@file:///ex.html:7:6\nglobal code@file:///ex.html:12:2",
>+                "global code@file:///ex.html:12:2\na@file:///ex.html:10:6\nb@file:///ex.html:7:6",
>+                "global code@file:///ex.html:1:1\nmap@[native code]",
>+            ];
>+
>+            for (let string of strings)
>+                InspectorTest.expectThat(WI.StackTrace.isLikelyStackTrace(string), `${JSON.stringify(string)} should be a stacktrace.`);
>         }
>     });
> 
>     suite.addTestCase({
>-        name: "testFormattedStrings",
>-        test(resolve, reject) {
>+        name: "WI.StackTrace.isLikelyStackTrace.InvalidStackStrings",
>+        async test() {
>             const strings = [
>                 "video:1:2",
>                 "video/mp4:1:2",
>@@ -106,11 +91,9 @@ function test()
>                 "http://video/mp4:1:2",
>                 "http://video/mp4 : 11:22:33-44:55:66",
>             ];
>-            
>-            for (let string of strings)
>-                InspectorTest.expectThat(!WI.StackTrace.isLikelyStackTrace(string), `"${string}" should not be a stacktrace.`);
> 
>-            resolve();
>+            for (let string of strings)
>+                InspectorTest.expectThat(!WI.StackTrace.isLikelyStackTrace(string), `${JSON.stringify(string)} should not be a stacktrace.`);
>         }
>     });
> 
>@@ -119,8 +102,6 @@ function test()
> </script>
> </head>
> <body onload="runTest()">
>-<p>
>-Test stack trace detection heuristic.<br>
>-</p>
>+<p>Test stack trace detection heuristic.</p>
> </body>
> </html>
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index ecd127d280b..9c34e79a609 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-09-11  Joseph Pecoraro  <pecoraro@apple.com>
>+
>+        Web Inspector: fix test case failures in js-isLikelyStackTrace.html
>+        https://bugs.webkit.org/show_bug.cgi?id=180664
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * UserInterface/Models/StackTrace.js:
>+        (WI.StackTrace.isLikelyStackTrace):
>+        In a quick benchmark 50% of the time was rebuilding the same complex regular
>+        expression over and over again. Instead just build the regex once and reset
>+        it before each use.
>+
> 2018-09-11  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Web Inspector: Fix typo "vritualized"
>diff --git a/Source/WebInspectorUI/UserInterface/Models/StackTrace.js b/Source/WebInspectorUI/UserInterface/Models/StackTrace.js
>index 1dfea88506b..5b0f4fe25c8 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/StackTrace.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/StackTrace.js
>@@ -80,12 +80,16 @@ WI.StackTrace = class StackTrace
>         if (/^[^a-z$_]/i.test(stack[0]))
>             return false;
> 
>-        const reasonablyLongProtocolLength = 10;
>-        const reasonablyLongLineLength = 500;
>-        const reasonablyLongNativeMethodLength = 120;
>-        const stackTraceLine = `(global code|eval code|module code|\\w+)?([^:]{1,${reasonablyLongProtocolLength}}://[^:]{1,${reasonablyLongLineLength}}:\\d+:\\d+|[^@]{1,${reasonablyLongNativeMethodLength}}@\\[native code\\])`;
>-        const stackTrace = new RegExp(`^${stackTraceLine}([\\n\\r]${stackTraceLine})+$`, "g");
>-        return stackTrace.test(stack);
>+        if (!WI.StackTrace._likelyStackTraceRegex) {
>+            const reasonablyLongProtocolLength = 10;
>+            const reasonablyLongLineLength = 500;
>+            const reasonablyLongNativeMethodLength = 120;
>+            const stackTraceLine = `(global code|eval code|module code|\\w+)?([^:]{1,${reasonablyLongProtocolLength}}://[^:]{1,${reasonablyLongLineLength}}:\\d+:\\d+|[^@]{1,${reasonablyLongNativeMethodLength}}@\\[native code\\])`;
>+            WI.StackTrace._likelyStackTraceRegex = new RegExp(`^${stackTraceLine}([\\n\\r]${stackTraceLine})+$`);
>+        }
>+
>+        WI.StackTrace._likelyStackTraceRegex.lastIndex = 0;
>+        return WI.StackTrace._likelyStackTraceRegex.test(stack);
>     }
> 
>     static _parseStackTrace(stack)

Actions: View | Formatted Diff | Diff

Attachments on bug 180664: 349439 | 349441 | 349444 | 349490