1654696 - js::GetCodeCoverageSummary always hits release assert

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P5)

Description

[Philip Chimento [:ptomato]]

For js::GetCodeCoverageSummary() to work, code coverage has to have been enabled before the first JSRuntime is created, or you hit a release assert. However, using only the embedder API there is no way to enable it other than an environment variable, so you can end up in a situation where calling js::GetCodeCoverageSummary() will always crash.

The JS shell has access to js::coverage::EnableLCov() which fixes this problem, but other embedders do not.

I propose adding a js::EnableCodeCoverage() in jsfriendapi.h next to js::GetCodeCoverageSummary() which calls js::coverage::EnableLCov() in its implementation.

Comment 1

[Jeff Walden [:Waldo]]

This seems like a suitable API to put in js/public/experimental in a new CodeCoverage.h header. The sort of thing that can be public, but might change in its exact operation or interface over time. All the code-coverage APIs could move there, even.

Comment 2

[Nicolas B. Pierron [:nbp — off until 29-09]]

(In reply to Philip Chimento [:ptomato] from comment #0)

For js::GetCodeCoverageSummary() to work, code coverage has to have been enabled before the first JSRuntime is created, or you hit a release assert.

This sounds like a bug, knowing that this code coverage was meant to be enabled by the dev-tools later on at runtime.
Have you tried using the Debugger API? See Debugger.collectCoverageInfo and Debugger.Script.getOffsetsCoverage().

I propose adding a js::EnableCodeCoverage() […]

This sounds good to me.

Comment 3

[Philip Chimento [:ptomato]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)

Have you tried using the Debugger API? See Debugger.collectCoverageInfo and Debugger.Script.getOffsetsCoverage().

Yes, but js::GetCodeCoverageSummary() is much more compelling because it returns a string already in the format I need :-)

Comment 4

[Evan Welsh [:ewlsh]]

Attachment: Bug 1654696 - Implement code coverage JSAPI. r=nbp,ptomato,jwalden

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1283143e3d55
Implement code coverage JSAPI. r=nbp,jwalden

Comment 6

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/1283143e3d55

Comment 7

[Philip Chimento [:ptomato]]

Comment on attachment 9167791 [details]
Bug 1654696 - Implement code coverage JSAPI. r=nbp,ptomato,jwalden

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Without this patch, the js::GetCodeCoverageSummary API is unreliable for SpiderMonkey embedders, and will hit a release assert in many cases, because code coverage now requires a flag to have been set that is not available as a public API for embedders. This patch adds a public API that embedders can use to set that flag.
• User impact if declined: Embedders must either lose code-coverage functionality or rely on an environment variable always being defined. In practice, this means that Linux distributions will need to carry this patch themselves.
• Fix Landed on Version: 81
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is trivial, only wrapping an internal API inside a public one, so it doesn't seem risky. However, the patch adds a new header file. If necessary, we can upload a separate version of the patch that only adds the embedder API in an existing header file instead of adding a new one.
• String or UUID changes made by this patch: None

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backporting something tagged as experimental makes me a bit uneasy, but I guess this doesn't affect browser behavior and probably isn't a huge deal in the grand scheme of things? Am I understanding that correctly, Waldo?

Comment 9

[Evan Welsh [:ewlsh]]

Attachment: 0001-Bug-1654696-Implement-code-coverage-JSAPI.-r-nbp-pto.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Embedders are currently unable to use the JSAPI coverage APIs in mozjs78 because they assert when coverage is not enabled. The API to enable to coverage is currently internal, this patch exposes that API.
Fix Landed on Version: 81
Risk to taking this patch (and alternatives if risky): None, does not impact browser code and only exposed any existing internal API.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 10

[Evan Welsh [:ewlsh]]

I wouldn't recommend landing the patch directly because it relocates existing JSAPI calls per Waldo's earlier comments. I've uploaded a patch which only exposes js::EnableCodeCoverage and does not relocate the existing calls. It should also apply cleanly to the latest esr78 head.

I believe this is marked experimental because it is exposing an experimental API? The functionality already exists, embedders just don't have access to it in mozjs78 (they previously did).

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9169227 [details] [diff] [review]
0001-Bug-1654696-Implement-code-coverage-JSAPI.-r-nbp-pto.patch

Nicholas, can you please give this a quick review and confirm that this looks good for ESR uplift?

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 9169227 [details] [diff] [review]
0001-Bug-1654696-Implement-code-coverage-JSAPI.-r-nbp-pto.patch

Review of attachment 9169227 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review. This is OK for ESR78.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9169227 [details] [diff] [review]
0001-Bug-1654696-Implement-code-coverage-JSAPI.-r-nbp-pto.patch

Thanks, Jason. Approved for 78.2esr.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/c26576cd0d55