Fix VFS CfAPI Syncroot Register Error.

[allexzander]

Signed-off-by: allexzander [email protected]

[allexzander]

Found the reason for the CfAPI wrapper to fail the Sync Root registration when using a build from the installer. Weird, but, this is caused by passing parameters by reference between different libraries. This problem appeared after adding a few more such parameters.

[nextcloud-desktop-bot]

AppImage file: Nextcloud-PR-3043-28771b25e371f032e2c953b50e34bde7ba922f92-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

[FlexW]

@allexzander Weird. Which libraries do you mean? Because registerSyncRoot() is just called in vfs_cfapi.cpp:66 which is also part of libsync. The only place where (in this part of the application) functions are called across dll boundaries is OCC::Utility::registrySetKeyValue(), OCC::Utility::registryGetKeyValue, OCC::Utility::registryDeleteKeyTree() and OCC::Utility::registryKeyExists(). Is it possible that we somehow do not export the functions correct?

[allexzander]

@FlexW Indeed, this could be the case, the weird thing is:

• the issue only happens when running a build from the "C:/Program Files" folder (confirmed this with multiple tests)
• the issue does not happen when using the X86 MSI (I guess it's the one you've been using)
• the issue only appeared after adding enough of reference-passed arguments

[allexzander]

@FlexW Well, I must add some edits to my previous comment. Unfortunately, this is due to "libsync" is a separate library, while, the GUI (where folder.cpp) resides, is not. If you look closely, the "folder.cpp" is not separated from the rest of the code and it gets built into .exe. While, "libsync" on the other hand, is a separate ".dll". So, yeah, the problem appears on the boundary between "libsync" and the rest of the code. It is not in the OCC::Utility.

Also, the failure happens when calling CfRegisterSyncRoot function, which is not related to registry values. Besides that, the Registry values always set successfully (confirmed by logging and checking the Registry contents after launching the client).

[allexzander]

@er-vin Any thoughts on this?

[er-vin]

@er-vin Any thoughts on this?

Won't have time to look in details, but I think @FlexW is right this is weird to do a "fix" here, I think you introduced a workaround but that the initial issue is still here and might crop up in some other way. The problem is likely somewhere else and at the boundary between gui and libsync indeed. I'd evaluate how things are passed to the vfs object for instance. Is it somehow bound to a temporary? This kind of things. And then walk all the way to where you did your fix. Right now the elements at hand are not informative enough to have a more precise opinion.

[allexzander]

@er-vin Indeed. This workaround, is, of course, is a temporary solution. Just to make us stick to the release plan. Frankly speaking, I never expected the Progress Bar feature to fail at this specific place. Everything looked fine. I'll be digging into a root cause later this week. And yes, the only DLL boundary we have right now is between GUI and libsync.

[er-vin]

Just to make us stick to the release plan. Frankly speaking, I never expected the Progress Bar feature to fail at this specific place.

And now you understand my surprise when that moved from "I got time I plan for 3.2.1" to "let's get it in now". It's easier to stick to a release plan when you don't move the goal post at the last minute. This often generate mistakes otherwise and you end up firefighting more than necessary (there were plenty of opportunities to do so with what was there already).

[allexzander]

@er-vin Yes. I learned a lesson. Yet, I doubt this would get caught during the review. It's so subtle. And is not happening on every Windows OS build (surprisingly :)).

[er-vin]

@er-vin Yes. I learned a lesson. Yet, I doubt this would get caught during the review.

Probably not, it's just a question of time you have to find the problems and fix them. Typically you were aiming for 3.2.1 but in my books that's something which was too big and more like feature improvement than true bugfix. I'd have earmarked it for 3.3.0 to be honest. If you wondered why I sound conservative sometimes it's why. ;-)

[er-vin]

Note that, if the progress is giving too much trouble, you still have the option to revert the merge commit which introduced it. That's mainly why the rebase + merge approach was introduced, taking a feature out of master is supposedly only a merge commit to revert. Might be a bit more complicated now due to the couple of "test PRs" which now pollute the history. But sometimes it's just better to make a step back to come better prepared later.

[allexzander]

Note that, if the progress is giving too much trouble, you still have the option to revert the merge commit which introduced it. That's mainly why the rebase + merge approach was introduced, taking a feature out of master is supposedly only a merge commit to revert. Might be a bit more complicated now due to the couple of "test PRs" which now pollute the history. But sometimes it's just better to make a step back to come better prepared later.

@er-vin Been thinking about that at the very beginning when the issue started to happen. But now, the temporary solution seems safe for now, so reverting everything will just bring more bad than good at this point.

[er-vin]

@er-vin Been thinking about that at the very beginning when the issue started to happen. But now, the temporary solution seems safe for now, so reverting everything will just bring more bad than good at this point.

Arguable. You use conditional to good effect ("it seems") while reverting would get you back to the known state of rc1 (and so less uncertainty since it was tested in the wild already).

[er-vin]

To clarify: I'm not telling you what to do and forcing your hand to revert. Just bringing more on the table to point out it's not necessarily as clear cut as you might perceive it at the moment (hamster wheel effect and all that).