Amphtml: Error: Attempting to access a non-existant slide

[dparikh]

Stack track:

Error: Attempting to access a non-existant slide
at Error (https://raw.githubusercontent.com/ampproject/amphtml/1527203409437/extensions/amp-carousel/0.1/slidescroll.js:554)
at showSlide_ (https://raw.githubusercontent.com/ampproject/amphtml/1527203409437/extensions/amp-carousel/0.1/slidescroll.js:593)
at (https://raw.githubusercontent.com/ampproject/amphtml/1527203409437/extensions/amp-carousel/0.1/slidescroll.js:485)
at getScrollTop (https://raw.githubusercontent.com/ampproject/amphtml/1527203409437/src/service/viewport/viewport-impl.js:486)
at measurePromise (https://raw.githubusercontent.com/ampproject/amphtml/1527203409437/src/service/viewport/viewport-impl.js:444)

Sample referrer:

Referrer
https://amp-wftv-com.cdn.ampproject.org/v/amp.wftv.com/www.wftv.com/traffic/incidents/driver-killed-another-hospitalized-after-possible-wrong-way-crash-on-i-4-in-orlando/758108607?amp_js_v=0.1&usqp=mq331AQECAEoAQ%3D%3D
User agent
Mozilla/5.0 (Linux; Android 7.0; BLN-L24 Build/HONORBLN-L24) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.158 Mobile Safari/537.36
Service
default-cdn-1p-canary
Version
001527816702194

[aghassemi]

also happening in current releases (973 and 656) but still fairly recent.

[aghassemi]

Only seems to happen in cases where carousel is initially hidden in a amp-lightbox does not seem to be causing user-visible bugs.

samples:
https://www-diezminutos-es.cdn.ampproject.org/v/s/www.diezminutos.es/famosos-corazon/famosos-espanoles/gmp12391/begona-gomez-mujer-pedro-sanchez/?amp_js_v=0.1&usqp=mq331AQECAEoAQ%3D%3D

https://www-dailymail-co-uk.cdn.ampproject.org/v/www.dailymail.co.uk/tvshowbiz/article-5794903/amp/Eamonn-Holmes-awarded-OBE-Queen-Elizabeth-dedicating-40-years-broadcasting.html?amp_js_v=a1&amp_gsa=1#ampshare=http%3A%2F%2Fwww.dailymail.co.uk%2Ftvshowbiz%2Farticle-5794903%2FEamonn-Holmes-awarded-OBE-Queen-Elizabeth-dedicating-40-years-broadcasting.html

/cc @cathyxz

[jpettitt]

Looks like a converter bug, I'll take it for now.

[aghassemi]

I have also not been able to repro the error locally, on desktop or mobile. same version of chrome as reported by errors...

[aghassemi]

I sort of expect this to be related to https://github.com/ampproject/amphtml/issues/15759

I seems like a race to me, amp-carousel calls getRealChildren to find and cache its child nodes, I suspect that's coming back empty in some cases causing going to slide 1 out of range. It's all speculation as I have not been able to repro this even on their sites.

[jpettitt]

I'm fixing the converter issues, I'll have that deployed in about 30 mins - however there is a weird effect where the amp-iframe in amp-carousel doesn't get out of the loading state until you've changed slides - I think this may be related to the data-block-on-consent. I'll update this as soon as the new config is live to confirm it's still happening.

[jpettitt]

Here is a repro url for the failing to render issue http://amp.wftv.com/www.wftv.com/traffic/incidents/driver-killed-another-hospitalized-after-possible-wrong-way-crash-on-i-4-in-orlando/758108607?_rmdebug=2

[jpettitt]

got it, it was some weird CSS in the original page. Fixed in the wftv case.

[aghassemi]

I have been able to reproduce this on (https://www-dailymail-co-uk.cdn.ampproject.org/v/www.dailymail.co.uk/tvshowbiz/article-5801343/amp/Kardashian-kids-birthday-North-West-Penelope-Disick-celebrate-year-donuts.html?amp_js_v=0.1&usqp=mq331AQECAEoAQ%3D%3D) . It happens sometimes when the the lightbox that contains the amp-carousel is closed. So seems related to unlayout.

I have also seen that carousel's UX break up badly a few times (in ability to go to next slide, etc..) which might be related as well.

this has been happening since last release (first seen May 28) so something from a week before that is the curlpit. This happens in previous versions of Chrome so not something Chrome 66 broke.

@cathyxz Let's work on this together and figure out what's happening. Setting back to P1

[cathyxz]

So the only thing that went in during that time is https://github.com/ampproject/amphtml/pull/14477. Everything else was documentation/validation/tests. The carousel scroll issue reproduces fairly consistently (like maybe 50% of the time), if you open / close lightbox or refresh enough times, you can cause it to get into an unscrollable state. I've only reproduced the non-existent slide error once. It occurred because the webpage above registers a tap to close the image lightbox, and somehow a scroll mistakenly triggered the tap. So it was trying to scroll to the next slide after the lightbox was closed. It's quite hard to reproduce this on desktop, but it doesn't actually cause visible errors on mobile.

Edit / Addition:
I don't think we properly handle slide changes when the amp-carousel is not laid out, and maybe that's what is causing this error. When we unlayout the amp-carousel (at least for type=slides), we set this.slideIndex_ = null. There may be places where we don't thoroughly check for this sufficiently. I guess we should also better account for the situation where a slide change is triggered on a carousel that is not laid out.

Anyway, seeing 2 separate issues that don't seem related to each other:

1. Non-existent slide error, triggered when a slide change happens after the amp-carousel goes through unlayout.
2. Carousel scroll snap issue. It actually behaves ok on iOS, but Chrome mobile can get into a very strange state where scrolling gets stuck in a very janky way. Repros maybe a little less than 50% of the time.

[aghassemi]

Also has been happening in the past: see https://github.com/ampproject/amphtml/issues/9467

[cathyxz]

Okay, I found a reliable repro for this bug. Swipe to change slide, and while that's happening, immediately press ESC to close the lightbox. This seems in line with my diagnosis above, and I think we can fix it by having better checks for unlayout. Filed https://github.com/ampproject/amphtml/issues/15822, for the carousel snap issue on Chrome. Will troubleshoot that separately.

[danielrozenberg]

This issue seems to have resurfaced in the last opt-in canary release (1534447274459) - @cathyxz @aghassemi can you PTAL and triage?

[aghassemi]

/cc @nainar newIndex is somehow coming as NaN on showSlide_. This is happening a lot. Should cherry pick.

[aghassemi]

@nainar @danielrozenberg this might be due to amp-carousel-scroll-snap and/or amp-carousel-new-arrows experiments flags. Those flags are only in Canary and not in Prod. So I am not too worried about this but I can't verify whether this is due to flags or code either.