Open Bug 1498277 Opened 1 year ago Updated 1 year ago

ScriptPreloader::CachedScript::GetJSScript(JSContext* cx) always returns null

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: jorendorff, Unassigned)

Details

The if-condition in this code is upside-down:

https://searchfox.org/mozilla-central/rev/1ce4e8a5601da8e744ca6eda69e782318afab54d/js/xpconnect/loader/ScriptPreloader.cpp#1229

If JS::DecodeScript succeeds, it returns JS::TranscodeResult_Ok, which is zero.[1][2] It's treated as false; the body of the if-block in GetJSScript is skipped; `script` is never stored in `mScript`; ScriptPreloader::CachedScript::GetJSScript returns null.

If JS::DecodeScript fails, it returns a non-zero value, but `script` is still null, so ScriptPreloader::CachedScript::GetJSScript still returns null.

In any case the error code is lost.

[1]: https://searchfox.org/mozilla-central/rev/1ce4e8a5601da8e744ca6eda69e782318afab54d/js/src/jsapi.cpp#7037

[2]: https://searchfox.org/mozilla-central/rev/1ce4e8a5601da8e744ca6eda69e782318afab54d/js/public/Transcoding.h#48
Actually ... this API seems like a usability hazard. The caller can't do much with TranscodeResult anyway, it seems to me. We should probably fix the API to behave like everything else: report any errors on cx and return a bool. The existing code here would magically become correct.

We need the capability to turn a TranscodeResult into a JS exception anyway for bug 1458209.

Moving to Core::JavaScript. :-|
Component: XPConnect → JavaScript Engine
(In reply to Jason Orendorff [:jorendorff] from comment #1)
> We need the capability to turn a TranscodeResult into a JS exception anyway
> for bug 1458209.

We do have that in the JS shell [1], but I found that matching enumerated error code had the benefit of not making any allocation and also to be easy to compare.

[1] https://searchfox.org/mozilla-central/rev/1ce4e8a5601da8e744ca6eda69e782318afab54d/js/src/shell/js.cpp#1849-1891
It doesn't always return null. The sync decode is only a fallback for when an off-thread decode result isn't already ready. But that case should still be fixed.
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.