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

RESOLVED FIXED in mozilla17

Status

()

Core
js-ctypes
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Irving, Assigned: Irving)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
Created attachment 651590 [details] [diff] [review]
Patch to add library path to error message

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-
Created attachment 652222 [details] [diff] [review]
Include file path in "failed to load library" error

(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+
Checked in to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/88def2d79ce8
https://hg.mozilla.org/mozilla-central/rev/88def2d79ce8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.