1686041 - Don't return generic NS_ERROR_FAILURE error code from nsLocalFile methods

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Simon Giesecke [:sg] [he/him]]

Some nsLocalFile methods, including IsDirectory and GetDirectoryEntries return a generic NS_ERROR_FAILURE error code on Windows, presumably originated from an unmapped Windows error code from https://searchfox.org/mozilla-central/rev/692a10e624a02fe79bf45046b586e50ea0ff17f1/xpcom/io/nsLocalFileWin.cpp#293. These errors currently cause a significant number of storage initialization failures as described in Bug 1482662.

To understand the cause of these issues, it would be helpful to map these to a value that allows to identify the original WIndows error code(s).

Comment 1

[Simon Giesecke [:sg] [he/him]]

In case there's no specific guess which Windows error code this might involve, we could add temporary telemetry that reports unmapped error codes. Nika, WDYT?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Using telemetry to report on unknown error codes seems like it could be reasonable.

The full set of error codes returnable by win32 APIs is very large (https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes), and so it is likely unreasonable for us to enumerate them all explicitly, but if there are specific high-frequency errors which we are not handling, we can definitely add them to the conversion function.

Comment 3

[Masatoshi Kimura [:emk]]

How about using NS_ERROR_GENERATE_FAILURE(FACILITY_WIN32 - NS_ERROR_MODULE_BASE_OFFSET, aWinErr)? This is compatible with HRESULT_FROM_WIN32.

Comment 4

[Simon Giesecke [:sg] [he/him]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #2)

Using telemetry to report on unknown error codes seems like it could be reasonable.

Ok. Unless we implement emk's suggestion, I can take care of that.

The full set of error codes returnable by win32 APIs is very large (https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes), and so it is likely unreasonable for us to enumerate them all explicitly,

Sure.

(In reply to Masatoshi Kimura [:emk] from comment #3)

How about using NS_ERROR_GENERATE_FAILURE(FACILITY_WIN32 - NS_ERROR_MODULE_BASE_OFFSET, aWinErr)? This is compatible with HRESULT_FROM_WIN32.

Interesting, I wasn't aware this option exists! That doesn't seem to be used anywhere else in the codebase though? Nika, what do you think about that, is that an option?

Comment 5

[Nika Layzell [:nika] (ni? for response)]

The only other place we use NS_ERROR_GENERATE_FAILURE is wrapping NSS errors into XPCOM errors, like we do here: https://searchfox.org/mozilla-central/rev/03224522336f60a1a61a87e1fcd4feb0a0315a9b/security/manager/ssl/NSSErrorsService.cpp#81. This dynamic error logic is a bit unfortunate, as it means that our nsresult enum doesn't exhaustively list all possible variants, but I suppose given that we already do it in one place it wouldn't be the worst thing to also do it here, at least on a temporary basis.

I would prefer not to use FACILITY_WIN32 - NS_ERROR_MODULE_BASE_OFFSET as the module, as we don't need to preserve nsresult compatibility with HRESULT here, and I would prefer to specify an explicit NS_ERROR_MODULE through ErrorList.py which is known to our other language bindings as well.

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1686041 - Map unknown Win32 API error codes to a module of its own in nsLocalFileWin. r=#xpcom-reviewers

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/151ba05d0328
Map unknown Win32 API error codes to a module of its own in nsLocalFileWin. r=xpcom-reviewers,nika

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Is there a canonical nsresult translation site that should potentially be updated? Ex: https://www.oxymoronical.com/nsresult/ (but it doesn't understand modules, I think? Looks like it just consults https://www.oxymoronical.com/nsresult/js/results.json)

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/151ba05d0328

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #8)

Is there a canonical nsresult translation site that should potentially be updated? Ex: https://www.oxymoronical.com/nsresult/ (but it doesn't understand modules, I think? Looks like it just consults https://www.oxymoronical.com/nsresult/js/results.json)

I've filed bug 1688671 on making searchfox support error results. And I think it turns out that https://james-ross.co.uk/mozilla/misc/nserror is better than the above.