googleads / videojs-ima

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

after calling player.dispose() videojs.ima crashes #1117

Closed damjan25 closed 9 months ago

damjan25 commented 1 year ago

After I try to fetch for ads I call player.dispose(); if none of the demand has ads After that videojs.ima crahes and goes into infinite loop throwing these 2 errors:

image

First time I saw this bug was today.

Browser: Chrome Desktop "videojs-ima": "^2.2.0" "@types/video.js": "^7.3.52",

damjan25 commented 1 year ago

it happens only on chrome v116! (tested with google test vast https://pubads.g.doubleclick.net/gampad/ads?iu=/21775744923/external/single_preroll_skippable&sz=640x480&ciu_szs=300x250%2C728x90&gdfp_req=1&output=vast&unviewed_position_start=1&env=vp&impl=s&correlator=)

It works on chrome (v115, v114), safari, edge, firefox.

before this happens I get this readyforpostroll warning in console (ONLY CHROME v116) and play() promise fails 4 times (player is muted) image

dioramayuanito commented 1 year ago

i'm curious too about graceful dispose of video.js . how about adding these lines before player.dispose() ?

player.ads.reset(); player.ima.controller.sdkImpl.reset(); player.dispose();

Kiro705 commented 1 year ago

Hello @dioramayuanito ,

I was testing this, but could not reproduce the issue.

Testing with: "video.js": "^8.3.0", "videojs-ima": "2.2.0" chrome: 116.0.5845.96

The test was done by calling player.dispose() during a preroll ad. No errors were encountered.

Would it be possible to share a code snippet or a video recording of the error. That may help me better reproduce the issue on my end.

Thank you, Jackson IMA SDK team

damjan25 commented 1 year ago

Thank you for your response guys.

@dioramayuanito I tried putting player.ads.reset(); before calling .dispose() but unfortunately still the same issue. I wanted to try the other one as well but I can not find on what I should call sdkImpl.reset(); on :/ for me this function name is undefined

@Kiro705 thank you for checking. I will try to make test env to reproduce this. Basically we have 2 preroll ads and then content after it. And after 1st ad is done playing and then calling .dispose() then videojs-ima starts permanently throwing these 2 errors above :/ And thank you sending me whole release notes list in other channel, I went through it and nothing seems to me that could be related to this. Could be that there were some changes directly in chrome v116 :(

MathkQ commented 10 months ago

Hi, @Kiro705 @damjan25 @dioramayuanito I have the same issue.

Working on: "video.js": "^8.6.0", "videojs-contrib-ads": "^7.3.2", "videojs-ima": "^2.2.0"

tested on: chrome: Version 118.0.5993.117 (Official Build) (x86_64) firefox: 115.3.1esr (64-bit)

In my case, the problem occurs because the method PlayerWrapper.prototype.setUpPlayerIntervals is set twice. Firstly when PlayerWrapper is initialized and secondly when PlayerWrapper.prototype.localContentEndedListener is executed.

I think the best solution for that will be to move

clearInterval(this.updateTimeIntervalHandle); clearInterval(this.seekCheckIntervalHandle); clearInterval(this.resizeCheckIntervalHandle);

straitly to the PlayerWrapper.prototype.setUpPlayerIntervals so from here to the begin of the method PlayerWrapper.prototype.setUpPlayerIntervals

Summarize: the problem is when the plugin override interval handlers without cleaning olds one.

damjan25 commented 10 months ago

I can confirm that this solves the issue described above.

MathkQ commented 10 months ago

@damjan25 for now you can solve this problem by executing this as a workaround: player.ima.controller.playerWrapper.playerDisposedListener(); before: player.dispose();

@Kiro705 can someone look at that PR?

rtkac commented 10 months ago

Hello, same issue here.

Workaround doesn't work for me: player.ima.controller.playerWrapper.playerDisposedListener(); before: player.dispose();

But I can confirm, that the solution to move clearIntervals to PlayerWrapper.prototype.setUpPlayerIntervals, mentioned by @MathkQ, worked as expected!

When can we please expect a new version of videojs-ima? cc @MathkQ @Kiro705

Thanks a lot!

Kiro705 commented 9 months ago

Hi all,

The changes are merged in https://github.com/googleads/videojs-ima/commit/75ecf39973fae646602de84f3855541fadd01e40. They should make it into the next videoJS release.