Iteration over dynamic NodeList exceeds updated length
Reported by
pi...@ideanest.com,
Aug 22 2016
|
|||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36 Steps to reproduce the problem: This error occurs rarely deep in the guts of Angular 1.4+. The code looks like this (paraphrased from https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L1902): function compileNodes(nodeList, [...]) { for (var i = 0; i < nodeList.length; i++) { // nodeList[i] is sometimes undefined here with i === nodeList.length [...] } } The nodeList when the error occurs is a dynamic NodeList obtained via element.children. It's hard to estimate the occurrence rate but at a guess it's less than 1 in 100000 executions of the function. What is the expected behavior? The loop should terminate when i has reached or exceeded the *current* nodeList.length. What went wrong? The nodeList's length decreases while iterating through the loop but the loop continues running, breaking its invariant. While code inside the loop can sometimes mutate the DOM, evidence suggests that it's not happening in this case and that Chrome is removing an arbitrary tail of nodes (mix of elements and whitespace text nodes) from the list. (Given the complexity of the code this is just a guess, though.) Did this work before? Yes This worked fine prior to Chrome v50. Chrome version: 52.0.2743.116 Channel: stable OS Version: 10.0 Flash Version: Shockwave Flash 22.0 r0 Please refer to https://github.com/angular/angular.js/issues/13960 for details on how we identified this issue and arrived at the conclusions above. The bug doesn't appear to be specific to an element type or a DOM structure. I've observed the error occurring in Chrome 50.0.2633.3 up to Chrome 53.0.2785.57, on all major platforms. It doesn't occur in other browsers.
,
Aug 24 2016
The issue is known, but there's nothing we can do without a repro :-(
,
Aug 25 2016
Why can't I view the duplicate issue 622103? I get "Reason: User is not allowed to view this issue", which is not terribly helpful.
,
Aug 26 2016
Issue 622103 is access restricted because it has internal stuff in it. You're not missing anything interesting -- essentially what it says is "this sometimes fails, but we have no good repro". The failure is so generic that it's pointless to even try to guess what's going on. It could be a bug in the specific application using angular. It could be a bug in angular itself. It could be a bug in Chrome. It could be an implicit dependency on internal behavior (iteration order, message loop or task queue details, etc) that changed. I'm willing to spend time debugging this, but with the data I have so far, I don't even know where to start. If you want to see this investigated, please provide a repro!
,
Aug 26 2016
We actually have a fair bit more data than you imply:
1. It's definitely a bug in Chrome. I confirmed that a for loop's condition evaluates to false right at the top of the loop, which breaks the JS spec. Basically:
for (var i = 0; i < nodeList.length; i++) {
assert(i < nodeList.length); // should always be true, but sometimes fails in Chrome
...
}
2. The bug was introduced in Chrome v50, no later than v50.0.2633.3, and has not yet been fixed as of v53. It's possible that the occurrence rate has gone down since v50, though.
3. When the error occurs, it does so soon after the page loaded: often in <700ms, and usually in <5000ms. (The pages where I saw the bug tend to stay loaded for tens of minutes, so this is not likely to be a sampling artifact.)
Based on the above and with no knowledge of Chrome internals, my guess is that the bug is due to an optimization introduced in v50 that affects for loops or NodeList.length evaluation, and that can break the loop invariant when it's introduced at just the wrong time. A less likely alternative guess is that there's a bug that causes nodes to be accidentally dropped from a NodeList (!?) but fails to update a cached NodeList length.
Were there any changes in v50 that might impact these areas?
I'm willing to spend time building a repro, but I need help forming a hypothesis that can guide me in discarding irrelevant code. Simply reloading my app in a loop is unlikely to be productive due to the long load times and the low occurrence rate (a single repro would likely take days to achieve running on one machine).
,
Aug 29 2016
Anything that is sort of reproducible can help. We'd be happy to have a least something shows the bug from time to time as a first step, refreshing the page over and over again seems just perfectly fine. Could you attach to this issue an example app that crashes? This way we can run it over night in several chrome tabs with a debugger attached. We can then start building on top of that, getting the tests to run faster is probably the main goal if we cannot provoke it often enough.
,
Aug 29 2016
It looks like even the home page of https://reviewable.io crashes every so often (even though there's very little going on there), so you can try putting this in a loop. It does open a connection to Firebase on load but I don't think looping it will trigger any abuse detection or waste too much resources -- I guess we'll see. If it crashes the page will put up a modal overlay with the phrase "Well that was…unexpected. Sorry—we got the crash report and we'll look into it right away." and make a connection to app.getsentry.com. Let me now if you need a more machine-friendly signal of a crash. Thanks. |
|||
►
Sign in to add a comment |
|||
Comment 1 by brajkumar@chromium.org
, Aug 23 2016