Closed Bug 1686041 Opened 4 years ago Closed 4 years ago

Don't return generic NS_ERROR_FAILURE error code from nsLocalFile methods

Categories

(Core :: XPCOM, task)

All
Windows
task

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox86 --- fixed

People

(Reporter: sg, Assigned: sg)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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).

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?

Flags: needinfo?(nika)

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.

Flags: needinfo?(nika)

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

(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?

Flags: needinfo?(nika)

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.

Flags: needinfo?(nika)
Assignee: nobody → sgiesecke
Status: NEW → ASSIGNED
Pushed by sgiesecke@mozilla.com: 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

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)

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
Blocks: 1688164

(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.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: