U4-10646 - Sorting nodes in edge cases can lead to blanking out of property data

Created by Sebastiaan Janssen 07 Nov 2017, 12:47:23 Updated by Will Hancock 23 Nov 2017, 11:04:11

Tags: Unscheduled

Subtask of: U4-9609

The sort dialog currently allows you to click the "Save" button as many times as you want. When you do this in rapid succession with a large number of nodes being sorted, it is possible that some property data gets saved as blank data.

The cause: as we are sorting (1st sort) we are updating content, so updating the cache, and then in the meantime we start reading from the cache (2nd sort) - bad things can happen.


Sebastiaan Janssen 07 Nov 2017, 12:49:53

First no-brainer commit: don't allow the Save button to be clicked more than once. https://github.com/umbraco/Umbraco-CMS/commit/87ace12a4f06ab1d147abd58fb6d961c550dd755

Sebastiaan Janssen 07 Nov 2017, 12:57:05

Looks like there's already a lock in the ContentService.Sort method though so I'm not sure now how this property data corruption could happen.

Sebastiaan Janssen 07 Nov 2017, 14:07:30

Aha, but nodeSorter.asmx.cs gets all the content items before it sends it off to the sort method, that's outside of the lock and could be getting partially populated caches.

Sebastiaan Janssen 07 Nov 2017, 16:13:26

I don't want to add an additional lock to nodeSorter.asmx.cs because we might end up in a deadlock eventually. It would be best to get the IContent items from a list of Ids in the locked ContentService.Sort method so that all data access is done within the lock and the data should not change will reading it.

So I added an additional method for this, which should be used everywhere. I guess we should obsolete the method that accepts an IEnumerable<IContent> as it is not safe?

I moved both usages of the old method to the new method.

PR: https://github.com/umbraco/Umbraco-CMS/pull/2314

Stephan 13 Nov 2017, 16:33:57


doing the whole thing in 1 place (content service) is indeed how it needs to be done, else anything bad can happen

Sebastiaan Janssen 14 Nov 2017, 06:20:36

Cheers! Merged up to 7.7.5 as well.

Will Hancock 23 Nov 2017, 11:04:11

Thanks @sebastiaan Do you know which version of the UI this has gone into? Just 7.7.5?

We reported this in our 7.5.14 - looking to upgrade in the new year now.

edit; Just read this is due for 7.7.5 and 7.6.12, so we won't see this in 7.5?

Priority: Major

Type: Bug

State: Fixed


Difficulty: Normal


Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.7.5, 7.6.12

Sprint: Sprint 72

Story Points:

Cycle: 5