1834224 - JS_TRY_* macros not usable in embedder code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Philip Chimento [:ptomato]]

The JS_TRY family of macros (https://searchfox.org/mozilla-central/rev/5d287de1f7e8618dcade5cc7d480e76fae99f79b/js/public/Result.h#123-174) use methods of JSContext. However, in embedder code, JSContext is an opaque type, so this won't compile. e.g.:

bug.cpp:18:5: error: member access into incomplete type 'JSContext'
    JS_TRY_BOOL_TO_RESULT(cx, JS::InitSelfHostedCode(cx));
    ^
/usr/local/include/mozjs-102/js/Result.h:130:26: note: expanded from macro 'JS_TRY_BOOL_TO_RESULT'
    if (!ok_) return (cx)->boolToResult(ok_); \
                         ^
/usr/local/include/mozjs-102/js/TypeDecls.h:28:22: note: forward declaration of 'JSContext'
struct JS_PUBLIC_API JSContext;
                     ^

Either these macros shouldn't be in the public headers, or we should have public API such as JS::BoolToResult(JSContext*, bool) etc. (or even a JS::Result<JS::Ok>(JSContext*, bool) constructor?) If you let me know which is preferred, I'm happy to contribute a patch.

Comment 1

[Jan de Mooij [:jandem]]

We're not sure if the JS::Result overhead is worth it for a lot of internal code. The cx->boolToResult method and JS_TRY_BOOL_TO_RESULT macro appear to be unused at this point too. We probably shouldn't add the JSAPI methods if we're not using this internally anymore.

Comment 2

[Philip Chimento [:ptomato]]

Attachment: Bug 1834224 - Remove JS_TRY_BOOL_TO_RESULT and JSContext::boolToResult. r?jandem

These are not used anywhere in the codebase, and cannot be used by
embedders because JSContext::boolToResult isn't public.

Comment 3

[Philip Chimento [:ptomato]]

Attachment: Bug 1834224 - Remove JS_TRY_OR_RETURN_NULL. r?jandem

JS_TRY_OR_RETURN_NULL isn't used anywhere in the codebase, and can't be
used by embedders because the JSContext::resultToBool method isn't
public.

Depends on D180555

Comment 4

[Philip Chimento [:ptomato]]

Attachment: Bug 1834224 - Move JS_TRY macros to internal header. r?jandem

These macros are not usable by embedders because they expand to
JSContext method calls which are not public. However, they are used in
several places in the codebase, so we need to keep them somewhere.
JSContext.h seems like a good place for that.

Depends on D180556

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef037dcb0e3e
Remove JS_TRY_BOOL_TO_RESULT and JSContext::boolToResult. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d52b70ee959d
Remove JS_TRY_OR_RETURN_NULL. r=jandem
https://hg.mozilla.org/integration/autoland/rev/8875630473bb
Move JS_TRY macros to internal header. r=jandem

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ef037dcb0e3e
https://hg.mozilla.org/mozilla-central/rev/d52b70ee959d
https://hg.mozilla.org/mozilla-central/rev/8875630473bb