JS_CODE_COVERAGE_OUTPUT_DIR returns too few sources with no empty code coverage when running firefox.

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Bug 1204554 works fine in the JS shell, but it does seems that only a few functions are being reported, and that among most of the functions being reported, the code coverage information got deleted before the serialization phase.

Apparently, one of the problem is that the source does not seems to be finalized first in the browser.  Which cause a lot of JSScript to be finalized without having any attached reports.
(Assignee)

Comment 1

2 years ago
Created attachment 8668899 [details] [diff] [review]
Record source filenames independently of the script coverage.

This patch modify the code such that it works even if the source objects are
finalized after the scripts. It works by always adding the source object or
reusing the existing one if it is already present.
Attachment #8668899 - Flags: review?(terrence)
Comment on attachment 8668899 [details] [diff] [review]
Record source filenames independently of the script coverage.

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

::: js/src/vm/CodeCoverage.cpp
@@ +349,5 @@
>      // Skip any operation if we already some out-of memory issues.
>      if (outTN_.hadOutOfMemory())
>          return;
>  
> +    // Lookup if there is already one entry.

This comment is either extremely ambiguous or flat-out disagrees with the source below it. The most probable meaning of this sentence, taken alone, is: "do a lookup if and only if exactly one entry exists". I could also generously interpret this sentence as saying: "lookup whether one and exactly one entry exists". But that also doesn't look right.

Maybe you mean: "Get the existing source annotations, or create a new one."

@@ +354,5 @@
> +    LCovSource* source = lookupOrAdd(comp, sso);
> +    if (!source)
> +        return;
> +
> +    // Write code coverage data into the allocated LCovSource.

s/allocated//

I don't think "allocated" adds anything here, given the null check above.
Attachment #8668899 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/8966326bc731
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.