[stable8.2] Fixes a possible infinite change-dir-loop #23984

Merged
merged 1 commit into from Apr 20, 2016

Projects

None yet

5 participants

@blizzz
Contributor
blizzz commented Apr 13, 2016

Those can occur when browsing in a mount that got unavailable

  1. Have an external mount with some directory levels
  2. Browse it
  3. Make the external mount unavailable, so it throws \OCP\Files\StorageNotAvailableException
  4. Click on a folder (can be in breadcrumb to) that exists on the mount

Without the fix the files app will infinitely attempt to switch between the clicked and the old folder.

Now, it redirects to /.

⚠️ there is no PR against master yet, my need is 8.2 currently

What ya think @icewind1991 @MorrisJobke @rullzer ?

rel https://github.com/owncloud/windows_network_drive/pull/372

@blizzz blizzz Fixes a possible infinite change-dir-loop
Those can occure when browsing in a mount that got unavailable
39004c2
@blizzz blizzz added this to the 8.2.4-current-maintenance milestone Apr 13, 2016
@blizzz blizzz added the green-ticket label Apr 13, 2016
@blizzz
Contributor
blizzz commented Apr 13, 2016

failing tests unrelated

@MorrisJobke
Member

What ya think @icewind1991 @MorrisJobke @rullzer ?

Makes sense. I will test later.

@karlitschek What about this backport?

@icewind1991
Member

Looks good 👍

@karlitschek
Member

great. please backport 👍

@blizzz
Contributor
blizzz commented Apr 15, 2016

Well, this is the 8.2 edition, it requires (forward) ports to 9.0 and master. But I guess this is implicit :)

@PVince81
Collaborator

Code looks good 👍

@blizzz for the 9.0 version you should check for the http status code 503, I believe this is already done there but would be good to check if it already works there.

@PVince81
Collaborator

👎 I tested this just in case and it doesn't work.

With SMB external storage the string "\Exception" is returned, probably because on 8.2 there was no "StorageNotAvailableException" thrown properly yet.
With WND I get "\OCP\Files\StorageNotAvailableException".

@blizzz are you sure about the double slashes ? Or is that an env thing ?

@PVince81
Collaborator

The way I tested it:

  1. Mount an SMB storage as "/smb" (with WND or SMB)
  2. Navigate into "/smb/test/sub" (and have another subdir "sub2" ready)
  3. Shut down SMB server
  4. Navigate into "sub2" subdirectory
@MorrisJobke
Member

Tested and works 👍

@MorrisJobke MorrisJobke merged commit af5d08d into stable8.2 Apr 20, 2016

21 of 23 checks passed

server-master-linux-php5.4-ci/database=sqlite,label=SLAVE Build #2063 failed in 6 min 22 sec
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Scrutinizer 1 new issues
Details
cla-bot-core Build #3063 succeeded in 6 min 7 sec
Details
core-ci-linux-jsunit/database=sqlite,label=SLAVE Build #60607 succeeded in 1 min 38 sec
Details
core-ci-linux-swift-primary-storage/database=mysql,label=SLAVE Build #54677 succeeded in 10 min
Details
core-ci-linux/database=mysql,label=SLAVE Build #29208 succeeded in 12 min
Details
core-ci-linux/database=oci,label=SLAVE Build #29208 succeeded in 23 min
Details
core-ci-linux/database=pgsql,label=SLAVE Build #29208 succeeded in 12 min
Details
core-ci-linux/database=sqlite,label=SLAVE Build #29208 succeeded in 8 min 37 sec
Details
ocs-api-integration-tests-ci Build #9489 succeeded in 1 min 32 sec
Details
server-master-linux-externals-ci/database=sqlite,external=smb-silvershell,label=SLAVE Build #9205 succeeded in 5 min 1 sec
Details
server-master-linux-externals-ci/database=sqlite,external=swift-ceph,label=SLAVE Build #9205 succeeded in 4 min 40 sec
Details
server-master-linux-externals-ci/database=sqlite,external=webdav-ownCloud,label=SLAVE Build #9205 succeeded in 6 min 37 sec
Details
server-master-linux-externals-smb-windows-ext-ci/database=sqlite,external=smb-windows,label=master Build #10857 succeeded in 6 min 15 sec
Details
server-master-linux-php7-ci/database=sqlite,label=SLAVE Build #37515 succeeded in 5 min 45 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=litmus,mirallBranch=v2.0.2,slave=SMASH Build #13559 succeeded in 7 min 24 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@0,mirallBranch=v2.0.2,slave=SMASH Build #13559 succeeded in 16 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@1,mirallBranch=v2.0.2,slave=SMASH Build #13559 succeeded in 24 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_shareLink,mirallBranch=v2.0.2,slave=SMASH Build #13559 succeeded in 22 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePermissions,mirallBranch=v2.0.2,slave=SMASH Build #13559 succeeded in 14 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationGroups,mirallBranch=v2.0.2,slave=SMASH Build #13559 succeeded in 18 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationInsideGroups,mirallBranch=v2.0.2,slave=SMASH Build #13559 succeeded in 26 min
Details
@MorrisJobke MorrisJobke deleted the fix-infinite-loop-in-unavailable-mount branch Apr 20, 2016
@MorrisJobke
Member

Tested and works 👍

Damn ... I just have seen the other comments now :( But I tested the code actually and it works here. I even stepped with the debugger throught the JS code. I used WND.

@MorrisJobke
Member

@PVince81 Let's test this together tomorrow before we revert this, because I just tested this locally and it worked. Also with the two slashes.

@MorrisJobke
Member

@blizzz are you sure about the double slashes ? Or is that an env thing ?

@PVince81 What browser do you use? I tested with Chrome.

@PVince81
Collaborator

I also tested with Chrome and WND from git, without debug config value.
Maybe different PHP versions render such strings differently ?

I have php5-5.6.20-1.2.x86_64.
Maybe checking for the substring "StorageNotAvailableException" without slashes would be good enough.

@MorrisJobke
Member

Maybe checking for the substring "StorageNotAvailableException" without slashes would be good enough.

So we revert this and fix it again? @blizzz You you please revert this and create a new PR with the fixes? Thanks

@PVince81
Collaborator

I'll take care of the follow up PR

@PVince81
Collaborator

Here we go #24144

@blizzz
Contributor
blizzz commented Apr 21, 2016

test it with https://github.com/owncloud/windows_network_drive/pull/372

anyway this construction is much error prone. Swiching dir and hopping it succeeds without any checks must fail in some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment