965043 - popcorn-wrapper.js uses eval() to create Popcorn instance

Status: RESOLVED INCOMPLETE

(Webmaker Graveyard :: Popcorn Maker, defect)

Description

[Jon Buckley [:jbuck]]

https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/src/core/popcorn-wrapper.js#L283-L429 is using string concatenation and `new Function()` (which is basically eval) to create a new Popcorn instance.

We'll need to replace this entirely with something that creates Popcorn instances programmatically.

Comment 1

[Alexander [:admix]]

Can you assign this one to me please, I'm trying to get closer to CSP implementation with all blocks fixed first.

Comment 2

[Alexander [:admix]]

I replaced 'new Function' with https://github.com/admix/popcorn.webmaker.org/commit/aeff88cfe0feba9f1b235d5c758c44aca467491b

Tested, looks like everything works fine.
Need info on that, please.

Comment 3

[Matthew Schranz [:mjschranz]]

I see it's time I finally got around to doing/helping with this. Reading/trying the patch.

Comment 4

[Matthew Schranz [:mjschranz]]

(In reply to Alexander [:admix] from comment #2)
> I replaced 'new Function' with
> https://github.com/admix/popcorn.webmaker.org/commit/
> aeff88cfe0feba9f1b235d5c758c44aca467491b
> 
> Tested, looks like everything works fine.
> Need info on that, please.

So this is only really working because of how you wrote the function but honestly isn't really ideal. I think a better understanding of what eval was doing before is in hand https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval.

To be short, what eval was doing for us before was taking a dynamic/changing popcornString (which doesn't actually have much "dynamic" to it anymore) and making that string an executable function that would create our popcorn instance. In the old days pre sequencer a Popcorn project only ever had one media so you couldn't really predict the source that was being given to construct the popcorn instance properly.

Long story short, things have changed a lot since this was originally written and I think the correct solution here is to simply make the Popcorn instance by hand. I would be more than happy to let you learn more about it all if you want to jump in on it Alex.

Comment 5

[Alexander [:admix]]

I would be really glad to get help on that and I would like to start going in details how it should be done in a proper way. Let me know where to start and what are the steps, so I can as soon as possible be on a same track. Thank you so much Matthew.

Comment 6

[Scott [:thecount] Downe]

It looks to me like the main issue is this area, starting on this line: https://github.com/admix/popcorn.webmaker.org/blob/aeff88cfe0feba9f1b235d5c758c44aca467491b/public/src/core/popcorn-wrapper.js#L248

That is creating a string of code, that is then later injected into the page. Instead of creating the script from a string and injecting it, we should instead have a js file of the script that is created, and fill it with variables containing the data, and instead of injecting it, we should require it in like the rest of popcorn.

It doesn't have to be a new script file though, it can be code that lives directly in an existing file, but the key is it has to be code, and not a string.

Example:

popcornString += "var popcorn = Popcorn.smart( '#" + target + "', " + url + popcornOptions + " );\n";

Should instead be something like:

var popcorn = Popcorn.smart( "#" + target, url, popcornOptions );

Then go through and find anything that was being added to the string and instead directly do what the string was doing.

This is likely a decent sized refactor from what I can tell.

Comment 7

[Matthew Schranz [:mjschranz]]

Yep, definitely a refactor. From what I can tell the code that was used before existed at a time when our Popcorn.smart calls wouldn't auto pick players.

Really though, there's no need for a string to be passed in and the createPopcorn function can simply manually be constructed based on data from the Media instance (duration/url, popcornOptions, etc).

Comment 8

[Matthew Schranz [:mjschranz]]

To start, I'll give you a quick explanation of how it currently is functioning.

It all starts at https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/src/core/media.js#L284 which only happens once on an initial project load/new project. Basically this is creating the popcorn instance. Some details on the significance of each piece of information being passed:

url - The source of the media being used for the Popcorn instance. This will always be what we call null video and will use our HTMLNullVideoElement wrapper.
target - The element we will do any injecting of DOM elements to. In our case we will always inject some sort of wrapper element before then dynamically adding the media itself.
popcornOptions - Basically any custom options defined in the config to be passed along upon the creation of that instance of Popcorn.
popcornCallbacks/popcornScripts - Both are dead code and can be ignored.

Basically, all this information eventually makes it's way to https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/src/core/popcorn-wrapper.js#L199 and that generated string is what's used with eval.

To be quick, you really only need to do https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/src/core/popcorn-wrapper.js#L248 and https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/src/core/popcorn-wrapper.js#L300 in javascript form inside the createPopcorn function of popcorn-wrapper.js.

Lot's of code can be removed, such as all (client-side) references to generatePopcornString. The createPopcorn function will wind up wanting to accept arguments similar to how generatePopcornString was before minus the now dead arguments.

Some pseudocode:

createPopcorn( popcornOptions, mediaUrl, target ) {
  var popcorn = Popcorn.smart( "#" + target, mediaUrl, popcornOptions );

  _popcorn = popcorn;
}

I'll leave the rest for you to figure out/dig. Feel free to hit up Scott or myself.

Comment 9

[Alexander [:admix]]

Thanks for the details Matthew, I already started working on this bug yesterday. I'll dig into it more and come up with the suggested solution soon.

Comment 10

[Alexander [:admix]]

Just to make sure that I'm on a right track.
Basically, what should be done is:
This lines: https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/src/core/popcorn-wrapper.js#L199-L315 should be rewritten from string concat method to regular javascript and brought inside createPopcorn(){} method here: https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/src/core/popcorn-wrapper.js#L320-L330

And one more: should I create this 'script' tag here https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/src/core/popcorn-wrapper.js#L323-L328 ?

Thank you.

Comment 11

[Matthew Schranz [:mjschranz]]

That's the basic gist of it. You don't need to worry about that script tag at all as it was never hit in the old code.

Comment 12

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/popcorn.webmaker.org/pull/602

Comment 13

[Chris DeCairos (:cade)]

Popcorn Maker is no longer under active development.

https://learning.mozilla.org/blog/product-update-for-appmaker-and-popcorn-maker