1164290 - ZipUtils.extractFiles() breaks with symbolic links

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[:gerard-majax]

STR:
 0. Produce a zip file that contains at least one symlink
 1. ZipUtils.extractFiles() this file

Expected:
 Extraction is completed successfully and we get proper symlinks created

Actual:
 Extraction aborts with promise error pointing to https://hg.mozilla.org/mozilla-central/annotate/617dbce26726/toolkit/modules/ZipUtils.jsm#l214

Problem is just ... the string used in the catch() block. Using target.permissions fixes this for me.

Comment 1

[:gerard-majax]

Attachment: Fix error code path of ZipUtils.extractFiles()

Jan, I'm asking you review because you landed this, but feel free to redirect
to anyone which is better suited :)

Comment 2

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bddb9ad6934

Comment 3

[:gerard-majax]

Including a symlink into the zip file used by xpcshell tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d82717a835ae

Comment 4

[:gerard-majax]

Including a symlink into the zip file used by xpcshell tests, and the fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=decc92d298f7

Comment 5

[Jan Keromnes [:janx]]

Comment on attachment 8604962 [details] [diff] [review]
Fix error code path of ZipUtils.extractFiles()

Review of attachment 8604962 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this! I must have missed that error when moving the code around. One nit.

Also, if you have a patch that adds a symlink to a test archive, and you feel it would be valuable, I'd be happy to review it as well.

::: toolkit/modules/ZipUtils.jsm
@@ +210,5 @@
>          try {
>            target.permissions |= FileUtils.PERMS_FILE;
>          }
>          catch (e) {
> +          dump("Failed to set permissions " + target.permissions.toString(8) + " on " + target.path + ":" + e + "\n");

If modifying `target.permissions` fails, this error message will be wrong. Please replace `target.permissions.toString(8)` with `FileUtils.PERMS_FILE.toString(8)`, or simply remove it (i.e. "Failed to set permissions on [...]").

Comment 6

[:gerard-majax]

Thanks. I do have a test that should do the job :)

Comment 7

[:gerard-majax]

test case, that should fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3aa3a35a1092

Comment 8

[:gerard-majax]

test case with the fix, that should pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6962ffccec7a

Comment 9

[:gerard-majax]

Attachment: Adding symbolic link into test zip file

Comment 10

[:gerard-majax]

and of course, symlink fails on windows ...

Comment 11

[:gerard-majax]

But we see the failures on linux that gets fixed :)

Comment 12

[:gerard-majax]

Attachment: Adding symbolic link into test zip file

And that should skip symlink testing on Windows ... :)

Comment 13

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c1ddb789e83

Comment 14

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #13)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c1ddb789e83

And I've forgot to commit :(

Comment 15

[:gerard-majax]

Attachment: Adding symbolic link into test zip file

Comment 16

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2a3b7a835a5

Comment 17

[:gerard-majax]

Attachment: Adding symbolic link into test zip file

Comment 18

[Jan Keromnes [:janx]]

Comment on attachment 8605296 [details] [diff] [review]
Adding symbolic link into test zip file

Review of attachment 8605296 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thanks for adding the symlink to the tests.

::: toolkit/modules/ZipUtils.jsm
@@ +210,5 @@
>          try {
>            target.permissions |= FileUtils.PERMS_FILE;
>          }
>          catch (e) {
> +          dump("Failed to set permissions " + FileUtils.PERMS_FILE.toString(8) + " on " + target.path + " (" + e + ")\n");

Nit: Maybe you don't need the "()" around the exception, because it will already surround itself with "[]":

(pid:23854) "Failed to set permissions 644 on /tmp/tmp2QJ34I/test_ZipUtils/test_extractFiles/zen/beyond_link ([Exception... "Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIFile.permissions]"  nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS frame :: resource://gre/modules/ZipUtils.jsm :: ZipUtils_extractFiles :: line 211"  data: no])"

::: toolkit/modules/tests/xpcshell/test_ZipUtils.js
@@ +32,5 @@
>    }
>  }
>  
> +function ensureHasSymlink(target) {
> +  // Just bail out if running on Windows, since symlink do not exists there.

Nit: "since symlinks do not exist there."

Comment 19

[:gerard-majax]

Attachment: Adding symbolic link into test zip file

Addressed the remaining nits.
We have green try on https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2a3b7a835a5

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a401d0b0573

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a401d0b0573