Browser crash when looking through old post activity in profile
-
Since discourse search is TRWTF, I decided to go looking through my old posts in an attempt to find an original bug report.
Repro steps:
- Load profile page
- Commence scrolling through old posts
- Keep scrolling . . .
- Notice it's getting slower?
- Notice that if you're a frequent poster, it's starting to get significantly slower by say, mid august?
- Yeah, keep going just a biiiit more.
- Crash.
Suspect #1? Bad partitioning + indexing on old posts and possibly poor handling of dom loading/unloading related to profile data.
CC @sam, @eviltrout
Feel free to impersonate me.
-
Confirmed secondary bug: The above bug also breaks discourse mouse cursor. It never changes to show interactable objects.
[edit] Clarification of the above secondary bug: Closing the window/opening a new window corrects the mouse, but if you re-navigate to what.thedailywtf.com from the crashed browser window using chrome the mouse cursor remains broken.
-
Tertiary bug: This also re-introduces the [en.topic_count.all] bug on the main topics list.
@PJH Can I have a 'hat-trick' bug badge?
-
-
;with badgers as ( select 'Hat-Trick' as badge )
select 'Hat-Trick' as badge from badgersAlternately,
;with badgers as ( select 'Hat-Trick' as badge )
select badge from badgers
-
-
Badge queries these days require copious amount of colons. I'm currently in the process of figuring out whether they're upper or lower.
And we still don't have
-
Wait, what? Show me an example of what you mean. That smacks of terrible design.
It should be something like
Badge name, badge sql - for each badge
-
Seemed appropriate.
-
I am pretty sure robin improved this in the past, but it may make sense to just add cloaked view and be done with the issue
-
-
Oh yes, I bought the single on iTunes.
-
an original bug report.
You did found an original bug to report. Can't have it all ...
-
Wait, what? Show me an example of what you mean. That smacks of terrible design.
Last clause on the
WHERE
.Edit: Text version of the query:
SELECT user_id, 0 post_id, current_timestamp granted_at FROM badge_posts WHERE topic_id NOT IN ( SELECT topic_id FROM badge_posts GROUP BY topic_id HAVING count(topic_id) <4 ) AND topic_id NOT IN ( 1000, 1673 ) AND ( :backfill OR user_id IN (:user_ids) OR 0 NOT IN (:post_ids) ) GROUP BY user_id HAVING count(*) >= 1024
-
Comments on the workrounds in there more than welcome by the way. I'm consciously aware of two, and they are required.
-
Can you explain what you're trying to do with '0 not in (:post_ids)?
Is that you giving a badge to everyone in the topic or something?
Might I also suggest that any time you run repeating, historical badge sql you create a minimum time or topic or post to look from? Either by selecting the previous run date, storing the value, or doing a select max(last_award_time/last_award_post) from database?
This will significantly reduce the work of badges by utilizing indexing.
-
Filed Under: There you go | Why am I reading those meta-posts? I dunno. I usually look for the ones you guys post in because they tend to get hillarious replies!
-
-
Or for those who prefer videos,
http://zippy.gfycat.com/InfatuatedConsiderateGhostshrimp.webm
-
Okay, it seems that the crash is due to an exponentially increasing number of concatenated strings being produced.
That doesn't sound like something they tell you not to do in Programming 101, does it?
-
That's more 300 level.
-
I managed to figure that kind of thing out while my age was still in single digits. I don't think I'm especially intelligent... and yet...
-
Doesn't that sound like something they tried to avoid before by unloading things from the DOM? Because I think we complained about that unloading and they tweaked it. Might have been broken at that point in time.
That doesn't excuse it, but I like to believe in the good in humans and software and stuff like that.Filed Under: I should probably start writing fanfiction!
-
They do the unloading of the DOM in threads but not activity because they never considered anyone would use it that much.
-
So obviously we are doing it wrongtm by posting too much. A civilized discussion should have no participant taking part in so many talks! (The talks will be forgotten / unloaded, though).
Filed Under: We should make a new account every time we hit the profile-crash-limit!
-
Exactly. No normal member of a community could possibly accrue that much activity and it's clearly artificially done to prove a point.
Or not.
-
I'd like to point out... there should be no reason for it to crash,with an exception to a possible out of memory exception. It should be stream/appending data, in a linear fashion. The way it exponentially increases in time to load to the point of a crash suggests anything other than linear. In fact, as ben has mentioned, it's exponential.
That shouldn't even be possible from a streaming perspective.
-
Yup, it's definitely not linear expansion.
It shouldn't be possible from a streaming perspective unless you're idiotic about grabbing large chunks of DOM, appending more and not playing proper release with your original (meaning that you end up bleeding memory more and more the more you do it)
-
Okay, it seems that the crash is due to an exponentially increasing number of concatenated strings being produced.
It's possible to do a number of things to decrease the cost of string building. In particular, it should be possible to keep the amount of work linear in the size of the resulting string (by using exponentially increasing memory allocations).
@ben_lubar said:That doesn't sound like something they tell you not to do in Programming 101, does it?
I wouldn't expect it to be covered until Data Structures and Algorithms, which is a bit later. (In my course, many years ago, it was in the 201 slot; they wanted everyone past the “what does print do lol?” stage first.)
-
Can you explain what you're trying to do with '0 not in (:post_ids)?
That's one of the workarounds. That parameter is required (it's checked for before running) otherwise the query won't be run. 0 NOT IN makes that part of the sub clause true, rendering the whole point of that parenthesised clause pointless. (:backfill is also checked for.)
Might I also suggest that any time you run repeating, historical badge sql you create a minimum time or topic or post to look from?
I'm checking to see if 'you've' posted more than 1024 posts everywhere except a few select places. How far back should I look? :)
-
Me? I would say probably 95% of my posts fall outside of 'those' topics. I'm not very active in 'those' topics.
But the check should have some type of persistence, otherwise queries like that get really horrible performance as time goes on.
-
It actually returns a list of users who've made more than 1023 posts. Anyone in that set, but doesn't already have the badge, gets it.
-
Except you have like 13 badges (or something) that all do essentially the same thing, with different thresholds.
Maybe something even as simple as
select user, '2^' + (sql power function / count(posts) + ' posts'
from topicposts
where bannedtopics in (x, y, z)
group by userRun that once daily, perpetual new badges for ever increasing values, no extra maintenance for you.
-
Except @PJH doesn't have the ability to create badges programmatically.
-
'2^' + (sql power function / count(posts) + ' posts'
And what's going to create the badge using that? I'd also need to specify the badge icon, badge level, bad explanatory text...
-
You're an inventive guy! Cron job, bash script, programmatic sql.
What could go wrong?
-
I don't have that sort of access to the server :)
-
Sure you do. It just takes one xss. We've already found several, I'm sure there are more.
-
By the way, check this out..
http://what.thedailywtf.com/my/notifications
I failed to crash scrolling all the way through mine on Meta:
-
I can't see that page. But you only have about 5K listed for "All" there, so I'm not surprised that you got through it.
-
It is available now and it is a welcome addition. Thanks @riking
-
Ah, nice. And that's where we go when we ask for older notifications. Seems like the "All" default thing should go away. It's just confusing.
-
Seems like the "All" default thing should go away. It's just confusing.
Sounds like a proposal that Jeff will like. :)
-
Could you add a filter-option for "unvisited notifications"? You know, the ones with blue background.
Filed Under: Just a thought
-
Sounds like a proposal that Jeff will like. :)
Let's see what happens:
-
Gee, that bug report looks familiar...
I'm guessing the response will be: "I don't use that page, the information is on other pages, so it doesn't matter that it's confusing, CLOSED!!!!"
Like last time.
-
Nope:
I specifically necro'd an existing related topic rather than started my own so there's some added context in order to shortcircuit a potential kneejerk TDWTF bug close.
-
Filed:
https://bitbucket.org/masamunewos/discoursebugs/issue/8/browser-crashes-when-navigating-through