eternaldensity / Sandcastle-Builder

xkcd: 1190: Time: The Game
Other
79 stars 65 forks source link

Non-functional without Google Analytics on Firefox 44 #1451

Open Zombie-Feynman opened 8 years ago

Zombie-Feynman commented 8 years ago

On Firefox 44, if the Google Analytics script cannot be loaded (e.g. because it is blocked with NoScript, as is my case), Sandcastle Builder is completely non-functional due to the following error:

TypeError: _gaq.push is not a function
    _gaq && _gaq.push(['_trackEvent', 'Load', 'Begin']);

A quick inspection in Firebug reveals that _gaq is of type

Object { __noSuchMethod__=function(),  _link=function(),  _linkByPost=function(),  more...}

which evaluates to true when used in a conditional:

_gaq && true || false;
true

To fix this, just replace _gaq && _gaq.push(...); with

typeof _gaq.push !== 'undefined' && _gaq.push(...);

wherever it occurs.

pickten commented 8 years ago

Alternatively, we could just provide a default _gaq={push:function(){}} before the script tries to load (or something of that sort).

eternaldensity commented 8 years ago

Alternatively, we could just provide a default _gaq={push:function(){}} before the script tries to load (or something of that sort).

I kinda assumed that it did something like this, but I guess not on firefox.

Zombie-Feynman commented 8 years ago

The strange thing is that it works fine with Firefox 43 and earlier. In fact, it may just as well be a Firefox bug, but I don't know enough about the JavaScript standard to tell.

friscoMad commented 8 years ago

Indeed the code already has a workaround for load errors like noscript in castle.js: var _gaq = _gaq || []; That is the code google offer to avoid errors like this. But it seems that _gaq exists and it is not an array nor has a push function so it seems more like an NoScript error loading an alternative script to analytics that do not behave correctly.

Could you open the console and call typeof _gaq;? Just to check at least whats the type of your _gaq objec

pickten commented 8 years ago

Judging from all this, I think it's either null or a string -- either of which is easy to test for. A one liner: if(typeof _gaq != typeof [] || !_gaq.push){_gaq={push:funtion(){}}}

(not [] because that's a bit slower since push would do actual work)

Zombie-Feynman commented 8 years ago

typeof _gaq reports "object". Also, typeof [] === typeof _gaq evaluates to true.

pickten commented 8 years ago

I'm too lazy to remember what typeof returns, hence the typeof [], but in any case, that essentially means the if catches and prevents this bug, since the issue was that _gaq.push is not a function (i.e. is undefined), and therefore falsy. I'll send a PR shortly, in that case, which should fix this and prevent other bad values from causing problems.

Zombie-Feynman commented 8 years ago

Redundacopypasta from my response to the pull request:

Nope, still the same error. It looks like the updated code somehow isn't being called before window.onload during normal execution. This causes the call to _gaq.push in Molpy.DefinePersist to trigger the error.

Oddly enough, setting breakpoints and stepping through the execution does appear to lead to _gaq being defined before Molpy.DefinePersist is called.