Adding workaround for browser bugs.

[peterfronc]

Its problematic as it is actually a valid code, however browsers are buggy and i have numerous reports for the exception occurring while loading script. I am quite sure it is not only coming with opentag but with wherever the uv-api is loaded.

Wrapping module variable check is required, feel free to adjust it to whichever format you prefer (ill pull it too).

@roberttod: Opentag will use immediately the fix but it will be good if you adjust it and merge to master asap.

I think you can put the uv object declaration separately and module independent, maybe detecting global object (pick global scope) - literally removing the modulePresent check ;-)

try {
  if (typeof module === 'object' && module.exports) {
    module.exports = createUv
  }
} catch (ex) {
}

[swagatha-christie]

By analyzing the blame information on this pull request, we identified @roberttod and @zaininfo to be potential reviewers

[KidkArolis]

This sound like it would break a lot of existing code out there? Chrome will probably fix this issue before it hits stable? Or maybe not...

[KidkArolis]

Works fine on Version 53.0.2785.34 beta (64-bit)

[KidkArolis]

the original code, I mean

[KidkArolis]

I don't think we need to apply this patch anymore.

[peterfronc]

I think I still need it, the problem is that i cant guarrantee noone will have fragile code in tag or libs loaded.

On 4 August 2016 18:11:16 BST, Karolis Narkevicius notifications@github.com wrote:

I don't think we need to apply this patch anymore.

---

You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/qubitdigital/uv-api/pull/24#issuecomment-237619008

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[roberttod]

I think if you just use

if (typeof module === 'object' && module.exports) {
  module.exports = createUv
}

rather than try/catch it will be fine.

[peterfronc]

Problem is that client code can have it and i cant stop them from doing it. Its better just to wrap it and be more secured.

On 4 August 2016 22:21:00 BST, Robert Tod notifications@github.com wrote:

I think if you just use

if (typeof module === 'object' && module.exports) {
 module.exports = createUv
}

rather than try/catch it will be fine.

---

You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/qubitdigital/uv-api/pull/24#issuecomment-237687188

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[peterfronc]

@roberttod it is not about opentag using try catch, it can be used by client code that is not set by us. For security reason its safer to wrap it. It cost nothing and I'll sleep better ;-)

[oliverwoodings]

I don't get how wrapping it in a try catch helps in that situation? I assume what you mean is if someone's website does this:

window.module = {
  exports: {}
}

Then our code would do:

module.exports = createUv

And you could access it via window.module.exports.createUv.

How would a try/catch help here?

[peterfronc]

Another thing is I'm not that sure that try/catch on preliminary code is only one causing it, clearly there is some dodgy optimization in chrome around module object and best it to wrap it up to spare on future problems.

[peterfronc]

@oliverwoodings wrapping the "typeof module" catches the exception (exception that should never occur) and library can load. That check normally fails, in recent chrome there is some bug that this check causes exception - it shouldn't but it does for some crazy reason. I prefer to have it wrapped just for sake of security - uv-api is included now with opentag.

[peterfronc]

Another problem is that - i am not sure what else can cause those optimizations to fail - clearly they are unstable.

[peterfronc]

@oliverwoodings try this in newest chrome (pick beta):

try{module.exports="test";}catch(c){};  if(typeof module==="object" && module.exports) {module.exports=q;}

[oliverwoodings]

@peterfronc i'm not talking about the chrome bug, i get what the chrome bug was. You said that it can be used by client code that is not set by us. I don't understand how this PR would stop a client messing with it.

[peterfronc]

Wrapping this check in try catch stops from exception being thrown, tags or any code added by client can contain some block like try catch to cause it again. Another thing is - with all unstable things - i dont know what other affecting problems there can be out there.

[peterfronc]

Its just about browser being unstable - we need to prevent it. We dont even know if we are not loosing any data now (I guess we don't ;) but you cant be sure now).

[peterfronc]

I understand try/catch is not beautiful - fully agree, it's just the breakage it can cause 🎱

[oliverwoodings]

The ability to do a type check on a variable and then assign a value to a property on it is a basic core concept of JavaScript. Are we going to start wrapping all other primitive JavaScript patterns in try/catch blocks too? This seems like a one off Chrome bug - as far as I can tell, they have already fixed it? The pattern of checking if module.exports exists is used in hundreds if not thousands of widely used JS modules - if a browser did cause that pattern to break, they would break thousands of apps and websites.

[KidkArolis]

The issue is in progress of being fixed: https://bugs.chromium.org/p/chromium/issues/detail?id=634467

[peterfronc]

It seems there is problem with module only. I know its weird, this does look like a problem with single script loads, it occurred and I prefer to have it wrapped. Most of modules you talk about Oli are loaded separately so it's not a problem, they are tested before releasing, in here its different story.

Maybe we will have no problem very soon as Karo suggests: https://bugs.chromium.org/p/chromium/issues/detail?id=634467

[peterfronc]

Lets have a look over few days, maybe we wont need any wrapping.