Issue metadata
Sign in to add a comment
|
Incorrect JS interpretation in v. 69-71 when using function on window instance
Reported by
jimmy.th...@gmail.com,
Dec 17
|
||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36 Steps to reproduce the problem: We have a fairly complex web application with functionality similar to excel, but with more complex cell editing controls. We are experience problems in Chrome v. 69, v. 70, and v. 71. Firefox and Safari works just fine, and so does older versions of Chrome. I'm not sure what causes this, and unfortunately I'm only able to reproduce it with our web application. But the screenshot should prove that there is in fact a problem, either with JS execution or Developer Tools. Please start with mark 1 and work your way to bullet mark 6. It will be obvious that Chrome is misbehaving. Our web application is built using ReactJS, Redux, and Fit.UI (https://fitui.org). What triggers the bug in Chrome v. 69-71 seems to be the use of a specific function in Fit.UI (again, works in all other browsers, even older versions of Chrome like version 65). Fit.UI features a mechanism for creating function overrides which allows the developer to invoke the original (overridden) function using base(). The base() function is temporarily registered on the window instance while the overridden function runs, and is automatically removed again. Example (from a function class) with pure JS: var orgGetDom = this.GetDomElement; this.GetDomElement = function() { // Do stuff return orgGetDom(); } The example above does not trigger the bug in Chrome. Example using Fit.UI which temporarily holds a reference to the overridden function on window.base. Remember, this works fine in all tested browsers except Chrome 69-71. this.GetDomElement = Fit.Core.CreateOverride(this.GetDomElement, function() { // Do stuff return base(); }); The implementation of Fit.Core.CreateOverride can be found here: https://github.com/Jemt/Fit.UI/blob/master/Core/Core.js#L122 The bug does not seem to be triggered when creating just a few rows at a time. But creating 50-300 new rows (with 7 complex editing controls in each row) triggers the bug almost every time, so it might be triggered by putting stress on browser. Since the debugger does not work properly (see the screenshot) we have no chance of resolving this problem ourselves. What is the expected behavior? Please see attached screenshot What went wrong? Please see attached screenshot Did this work before? Yes Works fine in e.g. Chrome 65 Chrome version: 71.0.3578.98 Channel: stable OS Version: OS X 10.13.6 Flash Version: I might be able to provide access to our application for debugging purposes. Please contact me.
,
Dec 17
I actually can't seem to reproduce the bug with Canary which is positive. But frankly, the reliability of Chrome and Developer Tools have been greatly declining for more than a year. What is going on? Please stop implementing new stuff and fix Chrome instead. Please don't turn Chrome into Internet Explorer.
,
Dec 17
,
Dec 17
,
Dec 18
Thanks for filing the issue... As per comment#0, it seems like the issue should be replicated using ReactJS, Redux, and Fit.UI which is out of scope for TE. Requesting someone from Blink>JavaScript team to look into the issue.
,
Dec 18
Thanks for looking into this. I'm sure this can be replicated using native JS, but I agree that it does require a serious effort to narrow down what's causing the problem. Let me know if access to our application is needed, although I can't guarantee it's possible. I would need permission to spend additional time and resources helping out during my work time.
,
Dec 19
@reporter: Could you please provide a sample file/URL that reproduces the issue, so that it would be really helpful in triaging the issue. Thanks.!
,
Dec 20
Hello again. First, let me add another interesting observation. Please see the two screenshots attached from Chrome v. 71. 1-Fails.png demonstrates that the code is misbehaving. That breakpoint should never have been reached. 2-Breakpoints-fixes-problem.png demonstrates that adding a simple conditional breakpoint in the debugger results in reliable execution. I have made numerous tests this way, and everything just works when the breakpoint is added. Regarding access to a running application demonstrating the problem: Is it worth the effort when the bug can not be reproduced with Canary? It will probably take us 3 days to establish a development server and set up the application. This is not a simple website, unfortunately, and we cannot give anyone access to the server I'm working on.
,
Dec 20
Thank you for providing more feedback. Adding the requester to the cc list. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 20
It would actually be more realistic for us to provide Remote Desktop access for an hour or so at an agreed time to do some initial debugging to confirm the bug. I suggest we use Chrome Remote Desktop. I would prefer doing this between 08:00-16:00 UTC or 19:30 - 23:00 UTC. Also, I will need an official @chromium.org account to which I can send the login details.
,
Dec 21
As per comment#10 from the reporter, could anyone from V8 team look into the issue and help in debugging. Thanks.!
,
Dec 24
The difference in behavior sounds related to I/TF.
,
Jan 2
Looking at the history with @neis, we believe this was fixed by https://chromium.googlesource.com/v8/v8/+/7b7e61c1e630572515c41c150e81bc904f793b4b. This would agree with the repro ranges (the bug was there from Chrome 69, fixed in 72). As a quick fix, could you try changing the CreateOverride function to not delete windows.base property? (That is, replace try-catch block at https://github.com/Jemt/Fit.UI/blob/master/Core/Core.js#L180 with window.base = undefined.) Does the bug reproduce with that change?
,
Jan 3
I believe you are right. I can confirm that replacing delete window.base; with window.base = undefined; does solve the problem. - Thank you :-) Jimmy
,
Jan 3
Would you recommend avoiding the use of 'delete' in general, and instead assign 'undefined'? Will it have any significant impact in V8?
,
Jan 3
Using 'delete' generally puts objects to slow mode, which does lead to worse performance for multiple reasons. If this code is very hot, you might want to avoid 'delete'. In this particular case, I would avoid 'delete' just to make the code uniform with the IE8 case.
,
Jan 3
That makes sense - thank you :-)
,
Jan 8
Removing Needs-Bisect as per C#13. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by jimmy.th...@gmail.com
, Dec 17