Roon Display Race Condition Freezes Album/Artist Images (ref#91P0JC)

[Marcell_Toth]

What’s happening?

· Other

Describe the issue

I sent a support request almost a year ago, that there is some race condition in the Roon Display site that will freeze the album/artist images at some random point and stop updating them. You can see my thread here: [https://community.roonlabs.com/t/roon-display-shows-random-unrelated-background-photos-ref-sm6kmb/267880](https://community.roonlabs.com/t/roon-display-shows-random-unrelated-background-photos-ref-sm6kmb/267880) You basically decided to ignore it, and due to "interesting" rules here, I'm unable to reopen my thread so I need to create a new one.

I got so annoyed by the lack of any support here that, being a web dev myself, I decided to investigate the solution myself. I don't usually do free work, but I found the solution and am happy to give the description of the problem and the exact patch to you (forfeiting all my IP) if only you fix this for good finally. Details in a comment below with screenshots.

Describe your network setup

Irrelevant, it's a bug in the display site's JS code

[Marcell_Toth]

Analysis

So here’s what’s happening. All you’re gonna see here is just me opening DevTools and analyzing display_ui.js.

So you have this crossfadeFeatureImage function. That has this nested blobUrlPromise.then callback, and then a nested doFinnaly which is the problem. The main point is doFinnaly does a swap again, but that happens sometime later, after the Promsie returned by blurArtistArt is resolved.

Now very importantly the then-callback closes on the back variable from the outer scope, but not the front one. Which means that when blobUrlPromise callback runs, the front div will be reevaluated, but not the back. Or more concretely, if we happen to do a swap, then another in a very short time, before blobUrlPromise resolves (this is most often the case if you go back to a previous track), the back variable will actually hold the now-front div (we’re after the swap). But front is running the jquery selector again, meaning that will also hold a reference to the very same div. You can see this in the screenshot, both variables have div.front.

Now if you swap a variable with itself, it’ll lose it’s class attribute, due to how that function is implemented, pretty self-explanatory:

Leading to the end result, where one of the divs don’t have a class attribute, this makes the other one stuck there permanently as any future swaps will not find .front.

Solution

Just put both variables in the closure. Or neither. Doesn’t matter, it’s the same 2 divs anyway. So either add this line, or drop the other one, too.

Ask

As you can tell from my opening post, while I’m generally a happy Roon customer, I’m very much skeptical about the level of support here, as so far I opened 2 requests, this one was ignored outright, the other one (playback failing) was investigated and confirmed, yet ignored again for more than half a year now.

I here gave you an exact solution for 100% free, that you literally just have to commit, QA, and publish. I really hope this will actually get through and get into a version sooner rather than later. Thank you!

Legal stuff

I allocate any intellectual property, including but not limited to any copyrights, regarding the above, including my explanation and code, to the public domain, free of charge, as-is, without a warranty of any kind.

[Suedkiez (Just a fellow user like you)]

@Marcell_Toth

You basically decided to ignore it, and due to “interesting” rules here, I’m unable to reopen my thread so I need to create a new one.

Just FYI, you can flag your old thread as „Other“ and ask the mods to reopen it. But they can merge them anyway. (It’s sad that it got ignored)

[Marcell_Toth]

Oh, thanks, that’s good to know.

[Marcell_Toth]

Some more tech advice

The above quick solution I offered you is just enough to stop it getting “stuck” forever, it doesn’t fix the core race condition, so sometimes Roon will still show the wrong artwork for a bit. (But at least it’ll clear itself on the next change.)

If you perhaps care to properly solve the problem, here’s some advice on how to do it. The issue is that you’re doing fire-and-forget promise continuations that depend on the current state of the DOM. That swap on line 23557 in the screenshot should only run if crossfadeFeatureImage hasn’t been called again since. Some options:

1. Make the entire crossfadeFeatureImage stack async, and queue invocations (e.g. write a wrapper) in a way where calling the next one will first await the last invocation. This is the elegant solution. Something like:

   Example wrapper creator

   export const sequentialExecution = <A extends any[], R>(
   innerFunction: (...args: A) => Promise<R>
   ): ((...args: A) => Promise<R>) => {
   	// This will hold the promise for the last function call (if any)
   	let previousExecutionPromise: Promise<R> | null = null;
   	return async (...args: A): Promise<R> => {
   		// append the new execution after the previous, or invoke it directly if this is the first one
   		previousExecutionPromise =
   			previousExecutionPromise !== null
   				? previousExecutionPromise.then(() => innerFunction(...args))
   				: innerFunction(...args);
   		return previousExecutionPromise;
   	};
   };
   

2. Instead of this whole swapping logic, you could keep appending DOM elements (and remove old ones after a timeout) and incrementing z-index, so any reordering of promise resolutions will not matter.

3. The lazy way is to just add a reordering-check. You either have a global / module variable, or an equivalent like a custom jQuery .attr('data-swap-order') on the parent element, which you always increment by 1 on line 23535. Then when you’re done with your async thing and want to modify the DOM again (line 23557) you first check if that attribute has changed since. If it did, you do nothing, it means another crossfadeFeatureImage is underway. If it’s still the same then you do the second swap.

   You kinda do something similar with that doSwap variable but 1. it’s not checked at the right time in case isArtist == true 2. you still update CSS even if it’s false, so it probably does something different, I did not investigate too much.

[daniel]

Hi @Marcell_Toth,
I’m sorry your previous thread got closed with no resolution. I’ve opened a ticket with our developers to look into this. Thank you for providing so many details. I cannot comment on a timeline for when a fix will be released for this but please bear with us. Thank you for your continued patience.

[Marcell_Toth]

Hi Daniel,

Thanks for the feedback, and actually getting some attention to this. I know how release cycles and sprints work, so I don’t have unrealistic expectations, but I hope my description is actually helpful and makes this a quick win that can be shipped somewhat soon.

[daniel]

Hi @Marcell_Toth
I’ve submitted a bug report with the dev team to make a fix for this and while I can’t comment on exactly when this will happen, it is in the queue. I don’t believe any other details are needed at this moment, so I’ll go ahead and mark this case to close.

Once the changes are ready, we will make a new build of Roon and mention the bug fix in our Software Release Notes section, so I would keep an eye out for improvements in in the next Roon release(s). Thanks!

[Marcell_Toth]

Thanks Daniel, thank you for handling this so professionally. I’ll be looking for the release notes, I always read them anyway:)

[daniel]

Thank you for providing such a detailed bug report—it was instrumental in helping us resolve the issue. I’m pleased we were able to address this to your satisfaction.