Closed Bug 1519200 Opened 4 years ago Closed 4 months ago

nsIFile returns different error codes if the file in question is already missing

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
101 Branch
Tracking Status
firefox101 --- fixed

People

(Reporter: mossop, Assigned: jkrause)

References

Details

Attachments

(1 file)

When nsIFile.remove is called for a path that is already missing we return different error codes depending on the platform. See bug 1519184 for an example of where that has bitten us.

OSX and Linux return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST. I haven't dug too deeply but I suspect it is a result of the unix implementation checking for the presence of a symlink first (https://searchfox.org/mozilla-central/source/xpcom/io/nsLocalFileUnix.cpp#1026).

Windows returns the probably more sane NS_ERROR_FILE_NOT_FOUND.

We should either make all return the same thing or at least audit existing uses of nsIFile.remove to make sure they are checking the return code correctly.

nsIFile.isDirectory also exhibits this issue, probably other methods too :(

Summary: nsIFile.remove returns different error codes if the file in question is already missing → nsIFile returns different error codes if the file in question is already missing
Priority: -- → P3

(In reply to Dave Townsend [:mossop] (he/him) from comment #1)

nsIFile.isDirectory also exhibits this issue, probably other methods too :(

Yep, directoryEntries is the same, too.

Was looking at taking this up, grepping the code shows places all over with checks like

if (rv == NS_ERROR_FILE_NOT_FOUND || rv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)

Was thinking though maybe it's technically correct to always check for both codes. What is the desired behavior here should we become consistent and always do NS_ERROR_FILE_NOT_FOUND. Or should it be more technically correct and return file target not exist in case of broken symlink and file not found in case that path is not symlink? If the later should windows return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST for broken shortcuts and win10 symlinks?

I didn't see anywhere that actually had different behavior for NS_ERROR_FILE_NOT_FOUND or NS_ERROR_FILE_TARGET_DOES_NOT_EXIST. Though did see many cases where only NS_ERROR_FILE_TARGET_DOES_NOT_EXIST was checked particularly in the nsJAR code not sure if those are intentional or not.

Thanks for thinking about helping!

This is Nathan's call but I'd argue that while arguably yes it would be more correct to return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST in the case of a broken symlink the vast majority of code doesn't care about this distinction and I don't think we should be forcing all caller to do extra work to verify the state. With this change it is slightly more work but still possible to verify that a path is a symlink and that its target exists.

Looks like the JAR code consistently uses NS_ERROR_FILE_TARGET_DOES_NOT_EXIST to mean that an entry doesn't exist in the zip file. I'm ambivalent about whether that needs changing.

Flags: needinfo?(nfroyd)

Thanks that's what made sense to me as well it seemed a bit much to expect all callers to check both

Assignee: nobody → fronkc1
Status: NEW → ASSIGNED

(In reply to Dave Townsend [:mossop] (he/him) from comment #4)

Thanks for thinking about helping!

This is Nathan's call but I'd argue that while arguably yes it would be more correct to return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST in the case of a broken symlink the vast majority of code doesn't care about this distinction and I don't think we should be forcing all caller to do extra work to verify the state. With this change it is slightly more work but still possible to verify that a path is a symlink and that its target exists.

I think this is the correct answer. I'm not sure what this does to error messages that might want to differentiate between the symlink and the actual target...but then again I'm not sure what our error messages look like for that anyway and if people are even considering that possibility in the first place.

Flags: needinfo?(nfroyd)

Also cleans up existing checks that were checking both file not found and target does not exist.

Attachment #9152913 - Attachment description: Bug 1519200 - Make nsLocalFile consistent with for unix and windows to return NS_ERROR_FILE_NOT_FOUND → Bug 1519200 - Make nsLocalFile consistent with for unix and windows to return NS_ERROR_FILE_NOT_FOUND. r=froydnj
See Also: → 1702416

(In reply to Nathan Froyd [:froydnj] from comment #6)

(In reply to Dave Townsend [:mossop] (he/him) from comment #4)

Thanks for thinking about helping!

This is Nathan's call but I'd argue that while arguably yes it would be more correct to return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST in the case of a broken symlink the vast majority of code doesn't care about this distinction and I don't think we should be forcing all caller to do extra work to verify the state. With this change it is slightly more work but still possible to verify that a path is a symlink and that its target exists.

I think this is the correct answer. I'm not sure what this does to error messages that might want to differentiate between the symlink and the actual target...but then again I'm not sure what our error messages look like for that anyway and if people are even considering that possibility in the first place.

Interestingly we even have NS_ERROR_FILE_UNRESOLVABLE_SYMLINK but I see no if or other explicit error handling associated with it. So I would not worry about symlinks at all.

But if this patch wants to move forward, it should probably also remove the definition of NS_ERROR_FILE_TARGET_DOES_NOT_EXIST itself to ensure we found all uses.

The bug assignee didn't login in Bugzilla in the last 7 months.
:nika, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: fronkc1 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(nika)

Jens, are you interested in taking over this patch? I'm not sure if this is still worth pursuing :-)

Flags: needinfo?(nika) → needinfo?(jstutte)

FWIW, I cannot really see why we would need to talk about the symlinks special case here given that it is currently mapped to ENOENT No such file or directory (POSIX.1-2001). which sounds much more like NS_ERROR_FILE_NOT_FOUND anyways. And if in practice nobody missed additional mappings here in the past I'd just stick with this.

Jan-Rio, would you want to take this over? It seems to be the right thing to just remove all uses and the definition of NS_ERROR_FILE_TARGET_DOES_NOT_EXIST in favor of NS_ERROR_FILE_NOT_FOUND. There is at least some instance here in bug 1720085 where we can see this inconsistency in the wild, too. I assume the patch needs some rebasing first. Thanks!

Flags: needinfo?(jstutte) → needinfo?(jkrause)

Sure, I will take a look.

Assignee: nobody → jkrause
Flags: needinfo?(jkrause)
Attachment #9152913 - Attachment description: Bug 1519200 - Make nsLocalFile consistent with for unix and windows to return NS_ERROR_FILE_NOT_FOUND. r=froydnj → Bug 1519200 - Remove `NS_ERROR_FILE_TARGET_DOES_NOT_EXIST` in favor of `NS_ERROR_FILE_NOT_FOUND`. r=#dom-storage-reviewers
Pushed by jkrause@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/430667732348
Remove `NS_ERROR_FILE_TARGET_DOES_NOT_EXIST` in favor of `NS_ERROR_FILE_NOT_FOUND`. r=xpcom-reviewers,nika,dom-storage-reviewers,jstutte
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 101 Branch
You need to log in before you can comment on or make changes to this bug.