Closed Bug 782471 Opened 7 years ago Closed 7 years ago

Add file name to JS error when ctypes.Create() fails

Categories

(Core :: js-ctypes, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: Irving, Assigned: Irving)

Details

Attachments

(1 file, 1 obsolete file)

When ctypes.open() fails, the error message does not include the full path of the library that failed to load. Adding this information helped me diagnose https://bugzilla.mozilla.org/show_bug.cgi?id=781628
The #ifdef flow feels a little dirty to me, but the mixture between control flow and platform-specific intermediate variables didn't leave an obvious alternative.
Attachment #651590 - Flags: review?(bobbyholley+bmo)
Comment on attachment 651590 [details] [diff] [review]
Patch to add library path to error message

How about making the argument a ternary value, conditional on libSpec.type, that evaluates to either libSpec.value.pathname or libSpec.value.pathname_u? As for the cleanup, we can just store the success of the JS_ReportError call in a local, and then return failures after doing unconditional cleanup.
Attachment #651590 - Flags: review?(bobbyholley+bmo) → review-
(In reply to Bobby Holley (:bholley) from comment #2)
> Comment on attachment 651590 [details] [diff] [review]
> Patch to add library path to error message
> 
> How about making the argument a ternary value, conditional on libSpec.type,
> that evaluates to either libSpec.value.pathname or libSpec.value.pathname_u?

The printf format string varies too; in the Windows case it's %hs to render the 16-bit filename string into an otherwise 8-bit output.

> As for the cleanup, we can just store the success of the JS_ReportError call
> in a local, and then return failures after doing unconditional cleanup.

I've rearranged the #ifdefs a bit, how does this look?
Attachment #651590 - Attachment is obsolete: true
Attachment #652222 - Flags: review?(bobbyholley+bmo)
Comment on attachment 652222 [details] [diff] [review]
Include file path in "failed to load library" error

I'd still prefer to avoid duplicating the JS_free call, but I don't think it's worth quibbling over. r+
Attachment #652222 - Flags: review?(bobbyholley+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/88def2d79ce8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.