search

googleads / videojs-ima

IMA SDK Plugin for Video.js
Apache License 2.0
450 stars 284 forks source link

Added null check #814

Closed mimse closed 5 years ago

mimse commented 5 years ago

We are seeing the same issue as reported here #728

image

I cannot figure out how this raise condition can happen when I look at the code, but it does happen. So to fix the issue I propose that we add a null check to the function and bail if the adsManager is null

gschoppe commented 5 years ago

Hi Mimse!

I'm not against this fix, but I'd love to get to the bottom of what's causing the erroneous call, as well.

I noticed from your screenshot that the vast majority of the errors are appearing on iOS. Is that in line with your general viewer stats, or is it likely that the error is more prevalent on these devices?

mimse commented 5 years ago

Hi @gschoppe

It seems to only affect Safari. But on all versions. Desktop / Mobil / Webview

I would also like to find out why it is happing.

I suspect it might have to do with an ad error and the timer not being cleared.

Kiro705 commented 5 years ago

Hi @mimse,

I was reviewing this issue. It looks like a race condition between the setInterval called function onAdPlayheadTrackerInterval and clearing the adManager.

Because there should be no ad UI to update without the adManager, I see this null check as a safe way to avoid this error.

Thanks for finding a solution, I'll be merging your pull request and plan to release a new version later this week.

Thanks, Jackson IMA SDK DevRel