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

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[:Irving Reid (No longer working on Firefox)]

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

Comment 1

[:Irving Reid (No longer working on Firefox)]

Attachment: 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.

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[:Irving Reid (No longer working on Firefox)]

Attachment: 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?

Comment 4

[Bobby Holley (:bholley)]

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+

Comment 5

[:Irving Reid (No longer working on Firefox)]

Checked in to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/88def2d79ce8

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88def2d79ce8