Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed fireEvent() in mraid.js. #942

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AdviewOpen
Copy link

we found some ads contents with mraid may remove EventListener in listeners function. for example:
function addVchange(){
mraid.isViewable()?viewChangeCB(true):mraid.addEventListener("viewableChange",viewCB);//add cb
mraid.addEventListener("viewableChange",viewCB2);//add cb2
}
function viewCB(n){
console.log('+++++++ viewCB() ++++++++');
mraid.removeEventListener("viewableChange", viewCB));//remove itself
}
function viewCB2(n){
console.log('+++++++ viewCB2() ++++++++');
}
so, if use currently mraid.js, in viewCB(), listener itself will be removed and next item will be null, that mean viewCB2() will not be triggered, we already found some ads contains like this usage, so we change the mraid.fireEvent(), we make a copy when events needs to be fired, if listener function remove itself, the fireEvent()'s array will not be cracked and keep its original order, then all of EventListners will be triggered correctly.
pls review it & check it ,greate thanks.

Copy link
Contributor

@YuriyVelichkoPI YuriyVelichkoPI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @AdviewOpen !

First of all - this is definitely a bug, and it's great that it was discovered and addressed.

However, I have two concerns here:

  • there is a risk of calling the same listeners twice if, due to some scenario the additional call of fireEvent will be made before the relation of the listener in another worker.
  • we need to be sure that this fix doesn't break the current behavior.

I'm not super strong in js so I would like to see your point about the first item, and if you think it makes sense, let's try to find a more robust solution without side effects for the existing flows.

For the second item, I'd like to ask @jsligh about auto tests. Have you tried to run this project https://github.com/prebid/prebid-mobile-autotests ? It contains hundreds of tests including the tests for MRAID ads. So we can use it to verify the current fixes. If we want to deprecate auto tests framework, it makes sense to migrate tests to the iOS and Android projects.

Pushing these changes to prod will be risky without appropriate testing since the changes will be shipped to all SDKs (apps).

@AdviewOpen
Copy link
Author

Hi,@YuriyVelichkoPI
Thx for your reply, and we just want to make a suggestion for integrity of prebid sdk, not a direction, so if your side can offer better solution to fix this issue, we'll glad to see and appreciate for your work.
And on android-sdk side, i also pull a request #732, the content is same as this one.
We hope the issue can be fixed soon & Great thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants