search

guardian / frontend

The Guardian DotCom.
https://theguardian.com
Other
5.84k stars 554 forks source link

JS App execution issue in Firefox 26 #3074

Closed kaelig closed 10 years ago

kaelig commented 10 years ago

In Firefox 26 & 27 Mac, the JS app seems to fail to load quite often.

How to reproduce

  1. Go to http://www.theguardian.com/uk/commentisfree?view=mobile in Firefox
  2. Open as many CiF stories as possible in the background (CMD+click on a link)
  3. Go through the tabs to find one where comments did not load

If you can't find any stories where the bug appeared, reload all tabs (right click on a tab, "Reload all tabs"). That usually triggers the bug.

Hints

On my computer, this seemed to happen mainly when a lot of tabs were loading at the same time.

In Firefox 27, a JS error is thrown: TypeError: d is undefined (not very helpful since app.js is minified).

HTML classes are js-on connection--not-low svg font-WebEgyptian-loaded font-WebAgateSans-loaded id--signed-out, showing that the scripts in the <head> are executed properly.

But the JS app does not seem to execute (no enhanced navigation menus, no comments…).

According to the network panel in Firebug, app.js is loaded.

screen shot 2014-02-10 at 18 40 11

Thanks to shinytoyrobots for pointing this to us on Twitter.

phamann commented 10 years ago

Seriously...

JS app seems to fail to load quite often.

is very different from:

when a lot of tabs were loading at the same time

I personally don't see this as a frontend related bug. Its most likely because Firefox's SpiderMonkey JS engine isn't multi-threaded nor do they use sandbox isolated tabs. Therefore opening 20 tabs will choke the engine.

kaelig commented 10 years ago

That's true, but then should we have a graceful degradation for when JS is On, the browser cuts the mustard, but the app does not load? It would mean that there still are links into place before content has been transcluded (ie a link to the discussion page)

phamann commented 10 years ago

Ok well the simplest solution to this would be to move this line inside the JS application. Although this would cause quite a strong FOUC. So its a trade off, I would prefer to keep it as is.

kaelig commented 10 years ago

Yes definitely. Still, it's weird that our JS fails like 10-20% of the time. According to @shinytoyrobots on Twitter, it happened during normal browsing activity.

DavidBruant commented 10 years ago

To find if it's caused by Firefox, try the STR in Safe Mode.

kaelig commented 10 years ago

Thanks @DavidBruant. I opened 20 tabs in Safe Mode, then when everything was loaded, I clicked "Reload All Tabs", and then all tabs had this JS error: TypeError: d is undefined.

Unfortunately I could not reproduce it yet on an environment where the JavaScript is not minified, as when running it locally, my machine is the bottleneck (not Firefox itself).

ahume commented 10 years ago

How come the links in the markup get removed if the JS app fails to load?

DavidBruant commented 10 years ago

In Firefox 28 (current Firefox beta), you can prettify the source code in the debugger, the same way you can in Chrome devtools. You can pause on uncaught exceptions in the debugger. It might help narrowing down what this d variable is all about. I haven't had the occasion to try yet, but you're even supposed to be able to set breakpoint in the prettified source. You might even be able to use the same trick than http://www.randomthink.net/blog/2012/11/breakpoint-actions-in-javascript/ to get console.log added to the minified source.

Overall, I recommend trying the new Firefox Devtools. In some areas, they've outgrown Firebug.

kaelig commented 10 years ago

@ahume In this case, if your browser cuts the mustard, then we (optimistically) hide the link to the discussion.

kaelig commented 10 years ago

@DavidBruant I used the beautifier in Firefox 28 Beta but unfortunately the error still points to the line of the minified JavaScript. That's not helpful at all. Any other idea?

ahume commented 10 years ago

@kaelig Yup, to avoid the flash you'd see I presume, makes sense.

Interesting point though, as ideally you wouldn't want to remove the link until even the content to include has successfully been loaded. Or write the link back in if it fails. Maybe a component could check at some point in the future whether it is successfully showing something, and if not it attempts a transclude request again, or shows the original link again. That doesn't solve your "JS app failed to load" scenario though.

In terms of pure resilience of the page, and access to content, I think it's dangerous to speculatively remove the link. Part of the beauty of the current load profile is that you get the core experience if stuff fails for any unknown reason. Hiding stuff upfront breaks this because it relies on there being no failures (in the JS app, the transclude request, the network, etc) before the content is eventually included. But I can see why you'd make that judgement call for "smoother" initial rendering, and less content flashes.

ahume commented 10 years ago

By the way, the fact you have a site where it's worth having this conversation is fucking amazing.

kaelig commented 10 years ago

Thanks Andy, I agree completely and I don't know why the discussion team has done it this way. It's a bit to optimistic and I'll raise an issue with them.

In the mean time, I'll try to investigate this Firefox bug further.

jamesgorrie commented 10 years ago

This isn't a discussion related issue, it's around how we avoid FOUC as @phamann mentioned.

Basically we have coupled discussion, and many other components, to the higher level solution to display / hide things based on JS availability.

We could however have a:

if (mustardIsCut) {
  foucIt();
}

$.on('breaking-error', unfoucIt);

The hard bit here is what is considered a breaking error?

We could obviousuly decouple this and say "hide the link once component has succeeded" - but then welcome to the world of the desktop site and being treated like a ping pong ball.

To sum up - I reckon our solution is sound. What we need to do is find out how to stop our app breaking (whether it's a bug with us or FF).

gidsg commented 10 years ago

Because discussion is at the bottom of the page isn't FOUC less of an issue?

kaelig commented 10 years ago

@gidsg I guess it can be an issue if someone has followed link to the comments section of an article, in which case this person will see a fouc.

jamesgorrie commented 10 years ago

Again - see the desktop site - it proves to be an issue there.

ahume commented 10 years ago

Yeah, you should totally fix the bug if there is one. :)

But the point remains. Failures are inevitable, whether they're browser bugs, network outages, users going in tunnels, mobile data providers mangling JS, CDN outages, etc... The site is built in a way that it generally deals with those failures really well (including not blocking rendering going to network for CSS, which I love).

The judgment call you've made on this one is to prioritise reducing fouc at the expense of resilience. Because, yeah, I hate the ping pong issue too. Although with that delay loaded multi-size ad unit at the top it's pretty much inevitable, right. ;)

Is your concern re fouc because you spend most of your time looking at the site in the office on a desktop machine with a fat broadband pipe? I spend 90% of my time on the site on a mobile phone on a train, so the things I care about are different. In fact, only content.

commuterjoy commented 10 years ago

Although with that delay loaded multi-size ad unit at the top it's pretty much inevitable, right. ;)

From user help when I asked them about their most common complaint ...

We usually receive a couple of emails a week about this and they're usually similar in tone to this one:

"As much as I love the Guardian, its website is among the more frustrating to try to read because of how "jumpy" it is -- scrolling around or clicking on stories, the page elements jump around in a stutter-y way too much before settling down, which makes the site not so pleasant a reading experience. I'd read more of the site, but I tend to get irritated with this slightly dizzy-making glitch, which constantly recurs. Most websites I look at do not have this problem. Anyway, just some technological feedback for you."

Worse than FOUC, in the user's eyes, is the jump. We should fix that too.

mattandrews commented 10 years ago

Just butting in here, but would it be worth having a URL parameter to load unminified/uglified JS source for debugging in prod this way? Or is that not worth the effort?

jamesgorrie commented 10 years ago

@commuterjoy: I've always seen FOUC and jump as the same thing, as hidden / visible is considered style.

@ahume: We don't want things to bounce around (see above). We could have the loading message for the component hidden on JS FOUC run, but perhaps also include the non-js link to the component if there is one. I suppose similar to "Download didn't work - here's the direct link".

This way:

if (mustardIsCut) {
  hideNonJsLinks()
  showLoadingWithNonJsLink()
}

I'll get on it.

@mattandrews: We should have a parameter that includes the source-mapped code. The reason we shouldn't include src JS is because you might come up against problems with compilation. I've been in situations where the src doesn't error, but the compiled code does. Source maps allow you to remain as close to prod as possible.

commuterjoy commented 10 years ago

@jamesgorrie Yes, they are the same. Jump is worse when you consider the variable height adverts attached to post page load events?

jamesgorrie commented 10 years ago

With that, could we not minimise it by having the ad container have the +-height of the advert?

kaelig commented 10 years ago

@jamesgorrie We already set the ad container to a min height that most adverts have, but some ad formats are bigger.

DavidBruant commented 10 years ago

@kaelig

the error still points to the line of the minified JavaScript

for the thrown error yes, but did you try "pause on exception" in the debugger? When the error occur, it should point to the exact place in the (prettified) code where the error occurs. You should be able to inspect the stack, variables, set breakpoints, etc. in the debugger seeing prettified source. Granted minified variable names aren't very expressive, but that might help.

jamesgorrie commented 10 years ago

@DavidBruant That would only be if we were serving source-mapped JS, which we aren't as including the source maps would up the file size for no benefit to the user.

DavidBruant commented 10 years ago

@jamesgorrie You can prettify without sourcemap. Granted, how useful that is is very contextual. What do you mean by "up the file size"? The source map and original sources are only fetched if the browser supports it and when devtools are open. Unless you're worried about the size of the sourcemap URL?

gidsg commented 10 years ago

<acronym type="soup"> Those who jump BTL before RTFA deserve a FOUC, IMHO. </acronym>

jamesgorrie commented 10 years ago

@gidsg Dangerous lands telling people how to read, never alone punishing them. @DavidBruant Sorry, out of date information and crossed wires, was thinking back to the days of source map information being included in the source files (I think Sass worked around browser issues like this for a bit).

mstorijo commented 10 years ago

Can we make a decision on this issue, please? Either to do something about it, or close.

kaelig commented 10 years ago

Could not reproduce in Firefox 28. Closing this.

mstorijo commented 10 years ago

:+1: