🚨 Error: Cannot read property 'forEach' of null

[ampprojectbot]

Details

Error report: link
First seen: Apr 25, 2019
Frequency: ~ 67/day

Stacktrace

Error: Cannot read property 'forEach' of null
    at forEach (extensions/amp-animation/0.1/runners/native-web-animation-runner.js:122:18)
    at pause (extensions/amp-story/1.0/animation.js:398:19)
    at pause (extensions/amp-story/1.0/animation.js:624:50)
    at forEach (extensions/amp-story/1.0/animation.js:624:23)
    at pauseAll (extensions/amp-story/1.0/amp-story-page.js:541:32)
    at setState (extensions/amp-story/1.0/amp-story.js:1985:21)
    at onPausedStateUpdate_ (extensions/amp-story/1.0/amp-story.js:766:11)
    at handler (src/core/data-structures/observable.js:78:6)
    at fire (extensions/amp-story/1.0/amp-story-store-service.js:573:29)

Notes

@renovate-bot modified extensions/amp-animation/0.1/runners/native-web-animation-runner.js:122-122 in #27350 (Mar 30, 2020)
@mszylkowski modified extensions/amp-story/1.0/animation.js:398-398 in #34466 (Jul 2, 2021)
@renovate-bot modified extensions/amp-story/1.0/animation.js:624-624 in #27350 (Mar 30, 2020)
@renovate-bot modified extensions/amp-story/1.0/animation.js:624-624 in #27350 (Mar 30, 2020)
@erwinmombay modified src/core/data-structures/observable.js:78-78 in #4249 (Sep 9, 2016)
@gmajoulet modified extensions/amp-story/1.0/amp-story-store-service.js:573-587 in #13997 (Mar 15, 2018)

Seen in:

• 07-03 Nightly (0008)

/cc @ampproject/release-on-duty

[ampprojectbot]

A duplicate error report was linked to this issue (link)

[rcebulko]

@mszylkowski Could you take a look at this? It seems like the pause method has a devAssert but not a devAssertArray. I'm not sure exactly why it's failing right now though. Seems to have been introduced (or at least re-triggered an existing bug) in #34466

[mszylkowski]

Ok so quick update: I was able to repro the bug, essentially when visibilityState=prerender&webview=1 and you start the story on a page with animations (example) and long press to pause (only works on mobile/devtools+mobile with reduced motion off), the error will show up. Still don't know why the devAssert doesn't catch the null value, but working on it.

[mszylkowski]

Seems like there was a try/catch that was catching this error before the change in #34466, even though the comments on the try/catch don't reflect the reasons why it was try/catching it. It was removed since the comment on those lines were fixed, but this brought the edge-case back, and since the try/catch was removed, they started surfacing.

https://github.com/ampproject/amphtml/pull/34466/files#diff-c5a86c80a77ae2a9abb4ac5ff1e9ef10f58c0e9262baa64273be1b4424d7161bL383-L388

We can re-introduce this try-catch and prevent these errors from surfacing, which is ok since the story is not initialized when the pause method is called.

An alternative is to change the behavior of pause by doing if (!this.players_) {return;} but it will alter the API. Another alternative is to call init before calling pause but it would be extra work for initializing the animations when users pause the story, which could introduce delays in the rest of the pausing code.

[rcebulko]

Thanks for taking care of this!