A pure function that should always return true sometimes returns false
Reported by
paulyoun...@gmail.com,
Apr 20 2018
|
|||||
Issue description
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.117 Safari/537.36
Steps to reproduce the problem:
The steps below are in the context of our full web application. We have yet to be able to produce a standalone demo. More detailed information is provided in the "What went wrong?" section.
1. Observe that the function sometimes returns false instead of true
2. Refresh the page
3. Observe that the function now always returns true
What is the expected behavior?
The function has a consistent return value.
What went wrong?
We got reports that users of our query language (running in JavaScript as part of our single-page web app) were sometimes seeing unexpected parse errors even though their expressions were valid. They also reported that refreshing the page "fixed" the issue in that they no longer saw parse errors for the same expression.
The minimal reproducible expression which should parse correctly (but not type-check) and sometimes did not is the single digit `"0"` (or any single digit character).
In the implementation of our query language parser, we depend on a function called `isDigit`. Upon investigation, we noticed that this function was sometimes returning `false` when it was expected to return `true` and were able to determine a set of steps to verify that the issue is always reproducible.
However, this behavior could only be verified using `console.log` statements. When using the debugger for the same set of steps, the code always behaved as expected.
When following our steps to reproduce, this code:
```javascript
console.log("isDigit(\"0\"): ", isDigit("0"));
```
Produces the following output in the console:
```
isDigit("0"): true
isDigit("0"): false
```
Our theory based on the observed behavior is that if the JavaScript file has not previously been loaded at least once then the issue occurs. Therefore, our steps to reproduce are:
1. Open a new Incognito window
2. Open the JavaScript console in the Chrome developer tools
3. Browse to our web application and log in
4. Navigate to the page in our application where queries can be entered
With the above steps, the issue is always reproducible.
Since refreshing the page "fixes" the issue, we have implemented the following workaround:
```javascript
// Chrome exhibits a bug where this function sometimes returns `true` (the
// expected behavior) and at other times returns `false`. We believe that this
// is _just one_ symptom of the bug, which has catastrophic implications for
// our query language parser. Specifically, this results in valid expressions
// being reported as invalid, with nonsensical error information being
// reported. The bug manifests itself in our application _at minimum_ when
// visiting the workbench when the browser does not have the JavaScript bundle
// cached. We have observed that refreshing the page "fixes" the issue (for
// some unknown reason) and that the issue remains "fixed" from then on.
// Rather than requiring the user to identify that they are experiencing the
// bug and then manually refresh, we detect the issue and reload the page.
if (isDigit('0') === false) {
window.location.reload(false);
}
```
We have confirmed that there are no naming conflicts or duplicted definitions for `isDigit`. Additionally, when copying the implementation of the (vendored) `isDigit` function to somewhere within our own source code we could not reproduce the issue but did start experiencing a different issue of a similar nature, as if the problem had "moved" to another function.
`isDigit` is implemented as:
```javascript
var isDigit = function (c) {
var diff = Data_Char.toCharCode(c) - Data_Char.toCharCode("0") | 0;
return diff <= 9 && diff >= 0;
};
module.exports = {
isDigit: isDigit,
};
```
`Data_Char.toCharCode` is implemented as:
```javascript
exports.toCharCode = function (c) {
return c.charCodeAt(0);
};
```
Did this work before? N/A
Chrome version: 66.0.3359.117 Channel: stable
OS Version: OS X 10.13.3
Flash Version:
Our web application is a closed-source, on-premises solution. With approval we may be able to make something available for people to reproduce this themselves.
We have been unable to reproduce this in Safari or any other browser which leads me to believe that this issue is specific to Chrome as opposed to V8 or something else shared between browsers.
We are happy to help however possible with this issue, including in person if that would help.
,
Apr 20 2018
Thanks for the help. I cannot currently reproduce in Version 68.0.3401.0 (Official Build) canary (64-bit) However, I also just came across this information in our internal bug tracker: Version 63.0.3239.132 (Official Build) (64-Bit) - reproducible Version 65.0.3323.0 (Official Build) canary (64-bit) - not reproducible Chrome Canary 66 (unspecified version) - reprodicible
,
Apr 20 2018
Good Version: 68.0.3401.0 Branch Base Position: 552221 Bad Version: 66.0.3359.117 Branch Base Position: 540276 python ./bisect-builds.py -a mac64 -g 552221 -b 540276 Downloading list of known revisions... (use --use-local-cache to cache and re-use the list of revisions) Downloading revision 545756...ac/97949// Received 79526904 of 79526904 bytes, 100.00% Bisecting range [540288 (bad), 552207 (good)], roughly 11 steps left. Trying revision 545756... Revision 545756 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: g Downloading revision 542867... Bisecting range [540288 (bad), 545756 (good)], roughly 10 steps left. Trying revision 542867... Revision 542867 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 544070... Bisecting range [542867 (bad), 545756 (good)], roughly 9 steps left. Trying revision 544070... Revision 544070 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: g Downloading revision 543623... Bisecting range [542867 (bad), 544070 (good)], roughly 8 steps left. Trying revision 543623... Revision 543623 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 543938... Bisecting range [543623 (bad), 544070 (good)], roughly 7 steps left. Trying revision 543938... Revision 543938 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: g Downloading revision 543727... Bisecting range [543623 (bad), 543938 (good)], roughly 6 steps left. Trying revision 543727... Revision 543727 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: g Downloading revision 543660... Bisecting range [543623 (bad), 543727 (good)], roughly 5 steps left. Trying revision 543660... Revision 543660 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 543683... Bisecting range [543660 (bad), 543727 (good)], roughly 4 steps left. Trying revision 543683... Revision 543683 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: g Downloading revision 543666... Bisecting range [543660 (bad), 543683 (good)], roughly 3 steps left. Trying revision 543666... Revision 543666 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 543673... Bisecting range [543666 (bad), 543683 (good)], roughly 3 steps left. Trying revision 543673... Revision 543673 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 543677... Bisecting range [543673 (bad), 543683 (good)], roughly 2 steps left. Trying revision 543677... Revision 543677 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b You are probably looking for a change made after 543677 (known bad), but no later than 543683 (first known good). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/171c0f9d4936c5f00b3cd5b920d3461df97f1cbf..1bfa23fdee284c09f24c0ea1ec805939a96daa27
,
Apr 20 2018
If you doublechecked the bisect results the only relevant change is r543682 "Update V8 to version 6.7.90" with 36426ab7388f8bea2ea218b4238075d7e886d0fb "[turbofan] Remove unsound SeqString types" Note, it was a fix for the bug. You can try finding the breakage too by bisecting 65-66 or an earlier range with 63.
,
Apr 20 2018
Double-checked with: python ./bisect-builds.py -a mac64 -g 543683 -b 543677 --use-local-cache --verify-range Downloading list of known revisions... Saved revisions 15734-552466 to /Users/py/.bisect-builds-cache.json Downloading revision 543677... Received 79032667 of 79032667 bytes, 100.00% Trying revision 543677... Revision 543677 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 543683... Trying revision 543683... Revision 543683 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: g Downloading revision 543677... You are probably looking for a change made after 543677 (known bad), but no later than 543683 (first known good). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/171c0f9d4936c5f00b3cd5b920d3461df97f1cbf..1bfa23fdee284c09f24c0ea1ec805939a96daa27
,
Apr 20 2018
No luck with 65-66. Will try an earlier range with 63. Good Version: 65.0.3323.0 Branch Base Position: 529554 Bad Version: 66.0.3359.117 Branch Base Position: 540276 python ./bisect-builds.py -a mac64 -g 529554 -b 540276 --use-local-cache --verify-range Downloading list of known revisions... Loaded revisions 15734-552466 from /Users/py/.bisect-builds-cache.json Downloading revision 529554... Received 78670158 of 78670158 bytes, 100.00% Trying revision 529554... Revision 529554 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Unexpected result at a range boundary! Your range is not correct. Cleaning up...
,
Apr 20 2018
Good Version: 60.0.3112.0 Branch Base Position: 474897 Bad Version: 61.0.3163.0 Branch Base Position: 488528 python ./bisect-builds.py -a mac64 -g 474897 -b 488528 --use-local-cache --verify-range Downloading list of known revisions... Loaded revisions 15734-552466 from /Users/py/.bisect-builds-cache.json Downloading revision 474901... Received 72796254 of 72796254 bytes, 100.00% Trying revision 474901... Revision 474901 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: g Downloading revision 488512... Trying revision 488512... Revision 488512 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 481820... Bisecting range [474901 (good), 488512 (bad)], roughly 12 steps left. Trying revision 481820... Revision 481820 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 478202... Bisecting range [474901 (good), 481820 (bad)], roughly 11 steps left. Trying revision 478202... Revision 478202 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 476293... Bisecting range [474901 (good), 478202 (bad)], roughly 10 steps left. Trying revision 476293... Revision 476293 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 475407... Bisecting range [474901 (good), 476293 (bad)], roughly 9 steps left. Trying revision 475407... Revision 475407 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 475253... Bisecting range [474901 (good), 475407 (bad)], roughly 8 steps left. Trying revision 475253... Revision 475253 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: g Downloading revision 475329... Bisecting range [475253 (good), 475407 (bad)], roughly 7 steps left. Trying revision 475329... Revision 475329 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: g Downloading revision 475366... Bisecting range [475329 (good), 475407 (bad)], roughly 6 steps left. Trying revision 475366... Revision 475366 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 475347... Bisecting range [475329 (good), 475366 (bad)], roughly 5 steps left. Trying revision 475347... Revision 475347 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 475337... Bisecting range [475329 (good), 475347 (bad)], roughly 4 steps left. Trying revision 475337... Revision 475337 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 475332... Bisecting range [475329 (good), 475337 (bad)], roughly 3 steps left. Trying revision 475332... Revision 475332 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: g Downloading revision 475334... Bisecting range [475332 (good), 475337 (bad)], roughly 2 steps left. Trying revision 475334... Revision 475334 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b You are probably looking for a change made after 475332 (known good), but no later than 475334 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/62f6039206aea40dbef07f171b5d70a3dcdb38b1..2a97a5d29f8b0518066e152d9f87ba5700954796
,
Apr 20 2018
Double-checked with: python ./bisect-builds.py -a mac64 -g 475332 -b 475334 --use-local-cache --verify-range Downloading list of known revisions... Loaded revisions 15734-552466 from /Users/py/.bisect-builds-cache.json Downloading revision 475332... Received 72829193 of 72829193 bytes, 100.00% Trying revision 475332... Revision 475332 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: g Downloading revision 475334... Trying revision 475334... Revision 475334 is [(g)ood/(b)ad/(r)etry/(u)nknown/(s)tdout/(q)uit]: b Downloading revision 475332... You are probably looking for a change made after 475332 (known good), but no later than 475334 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/62f6039206aea40dbef07f171b5d70a3dcdb38b1..2a97a5d29f8b0518066e152d9f87ba5700954796
,
Apr 20 2018
As per bisect logs above suspecting: broken in 14fa66b7a308d6c637f15cda07cb3f33a0b79fd3 "[turbofan] Add SeqStringCharCodeAt operation." fixed in 36426ab7388f8bea2ea218b4238075d7e886d0fb "[turbofan] Remove unsound SeqString types"
,
Apr 20 2018
BTW try a workaround without charCodeAt, for example: return c.length === 1 && c >= '0' && c <= '9'
,
Apr 21 2018
Thanks for the suggestion. I'm sure that will work in this specific case. I'm not sure I can confirm because, as I mentioned in the original bug report, even just copying the vendored `isDigit` function and using that caused the issue to move elsewhere. Also, I just checked our final JavaScript bundle and we have many instances of `charCodeAt` that I don't think we can work around in this way. I really appreciate the thought though.
,
Apr 24 2018
,
Apr 24 2018
I'm assuming this can be marked as Fixed since the suspected fix seems to have landed in M67, and thus there shouldn't be any further work needed?
,
Apr 24 2018
Wouldn't it be nice if the fix is merged to 66? Provided it's technically possible, of course.
,
Apr 24 2018
woxxom@, thank you so much for triaging so many crbug issues. bmeurer@, can you please check the above fix (https://chromium.googlesource.com/v8/v8/+/36426ab7388f8bea2ea218b4238075d7e886d0fb) need to be considered for M66 stable re-spin? and it's going to be really safe? atm i'm leaning towards M67 stable?
,
Apr 24 2018
I'd consider it safe, it's a straight-forward code removal. I'll leave it to jarin@ (TurboFan owner) to decide.
,
Apr 30 2018
I also think the fix is relatively safe to merge.
,
May 14 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by woxxom@gmail.com
, Apr 20 2018