See <https://www.tjhsst.edu/~jcranmer/test-ui/mailnews/mime/src/jsmime.jsm.html>, or the companion <https://www.tjhsst.edu/~jcranmer/test-ui/mailnews/mime/src/mimeJSComponents.js.html>. Some of those 0-line execution lines *definitely* are getting executed in tests. FakeTextDecoder is the best illustration of this: 1. There's only one place that constructor gets called, line 62 of that file. 2. That line reports execution count of 0. 3. The constructor is definitely executed, see line 25. Interestingly enough, I started checking the resulting LCOV coverage for a test that executed that line versus one that probably didn't: Record with test: TN:_5bSystem_20Principal_5d_2c_20resource_3a_2f_2f_2fmodules_2fjsmime_2ejsm SF:resource:///modules/jsmime.jsm FN:1,top-level FN:24,FakeTextDecoder FN:31,FakeTextDecoder.prototype._reset FNDA:1,top-level FNDA:4,FakeTextDecoder FNDA:4,FakeTextDecoder.prototype._reset FNF:3 FNH:3 BRDA:84,0,316,1 BRDA:24,0,19,4 BRDA:24,10,38,4 BRF:3 BRH:3 DA:17,1 DA:57,1 DA:70,1 DA:80,1 DA:83,1 DA:6,1 DA:15,1 DA:17,1 DA:30,1 DA:31,1 DA:41,1 DA:42,1 DA:57,1 DA:66,1 DA:70,1 DA:71,1 DA:78,1 DA:71,1 DA:80,1 DA:81,1 DA:80,1 DA:83,1 DA:85,1 DA:86,1 DA:85,1 DA:88,1 DA:89,1 DA:84,2 DA:25,4 DA:32,4 DA:34,4 DA:32,4 DA:35,4 DA:36,4 DA:38,4 DA:36,4 DA:39,4 LF:37 LH:37 end_of_record Record without test: TN:_5bSystem_20Principal_5d_2c_20resource_3a_2f_2f_2fmodules_2fjsmime_2ejsm SF:resource:///modules/jsmime.jsm FN:1,top-level FN:24,FakeTextDecoder FN:31,FakeTextDecoder.prototype._reset FN:41,FakeTextDecoder.prototype.encoding FN:42,FakeTextDecoder.prototype.decode FN:58,FallbackTextDecoder FN:71,top-level FNDA:1,top-level FNF:7 FNH:1 BRDA:84,0,316,1 BRDA:24,0,19,- BRDA:24,10,38,- BRDA:42,0,19,- BRDA:43,10,46,- BRDA:45,33,138,- BRDA:51,76,166,- BRDA:74,0,121,- BRF:8 BRH:1 DA:17,1 DA:57,1 DA:70,1 DA:80,1 DA:83,1 DA:6,1 DA:15,1 DA:17,1 DA:30,1 DA:31,1 DA:41,1 DA:42,1 DA:57,1 DA:66,1 DA:70,1 DA:71,1 DA:78,1 DA:71,1 DA:80,1 DA:81,1 DA:80,1 DA:83,1 DA:85,1 DA:86,1 DA:85,1 DA:88,1 DA:89,1 DA:84,2 DA:25,0 DA:32,0 DA:34,0 DA:32,0 DA:35,0 DA:36,0 DA:38,0 DA:36,0 DA:39,0 DA:43,0 DA:44,0 DA:45,0 DA:46,0 DA:47,0 DA:51,0 DA:52,0 DA:53,0 DA:59,0 DA:60,0 DA:61,0 DA:62,0 DA:72,0 DA:74,0 DA:75,0 DA:76,0 LF:53 LH:28 end_of_record Hmm, notice how the FNF and LF numbers are very different for those two records? It sounds like there's two records being generated for the file in some scenarios, and one's getting overwritten.

Thanks for the bug report :) (In reply to Joshua Cranmer [:jcranmer] from comment #0) > Hmm, notice how the FNF and LF numbers are very different for those two > records? It sounds like there's two records being generated for the file in > some scenarios, and one's getting overwritten. Indeed, having different FNF / LF on the same file does not makes sense, even to me. I have 2 guesses to explain these differences: (1) The parser does not honor the flag that is set to prevent lazy-script creation, and thus the JSFunctions scripts cannot be investigated. But in such case I would expect to have an assertion failure [1]. (2) The GC does not finalize scripts starting from the top-level. This would be unexpected, as the JSScripts / JSFunctions are being allocated linearly by the parser, and especially when the lazy-script parsing is disabled. And if so, I would expect that we cannot load the JSScript out of the JSFunction as its pointers should be poisoned. I guess the first hypothesis is the most likely, I guess running with a debug build should either trigger the assertion or returns correct code coverage (in which case this is still a bug). [1] https://dxr.mozilla.org/mozilla-central/source/js/src/vm/CodeCoverage.cpp?from=CodeCoverage.cpp#143

(In reply to Joshua Cranmer [:jcranmer] from comment #0) > […] It sounds like there's two records being generated for the file in > some scenarios, and one's getting overwritten. Hum … I especially made a different "TN:" section in order to be able to return the fact that one compartment (JS global) can load one JSScript which is loaded in another global. Having 2 TN: sections with the same files means that the file is loaded twice within different compartments, and normally genhtml should merge these instead of taking one instead of the other.

(In reply to Nicolas B. Pierron [:nbp] from comment #2) > Hum … I especially made a different "TN:" section in order to be able to > return the fact that one compartment (JS global) can load one JSScript which > is loaded in another global. Having 2 TN: sections with the same files > means that the file is loaded twice within different compartments, and > normally genhtml should merge these instead of taking one instead of the > other. They have the same TN:. (I'm not actually using genhtml, I have my own scripts that merge output files, but DA: counts are additive). I don't have a better testcase for you than "compile TB, run ./mozilla/mach xpcshell-test mail" (or, more specifically, mailnews/mime/test/unit/test_jsmime_charset.js is the test tickles this code, and just about any other test won't tickle the code).

(In reply to Joshua Cranmer [:jcranmer] from comment #3) > (In reply to Nicolas B. Pierron [:nbp] from comment #2) > > Hum … I especially made a different "TN:" section in order to be able to > > return the fact that one compartment (JS global) can load one JSScript which > > is loaded in another global. Having 2 TN: sections with the same files > > means that the file is loaded twice within different compartments, and > > normally genhtml should merge these instead of taking one instead of the > > other. > > They have the same TN:. This would mean then that the same file is loaded twice in the same compartment. Which is likely to also be a bug in ThunderBird then.

(In reply to Nicolas B. Pierron [:nbp] from comment #4) > This would mean then that the same file is loaded twice in the same > compartment. > Which is likely to also be a bug in ThunderBird then. I should explain more thoroughly. In two separate test runs, the same source file in the same compartment produces different FHF/LF values. In one of those runs, it's omitting lines of code that are run, which makes me think that something is up if code is run in the interpreter first and then via a JIT or via different JITs or something (I don't know the JS engine internals well enough to form a coherent hypothesis)--in short, I think there's multiple things generating code coverage stats for the same JSScript and they're not getting properly coalesced.

(In reply to Joshua Cranmer [:jcranmer] from comment #5) > In one of those runs, it's omitting lines of code that are run, which makes > me think that something is up if code is run in the interpreter first […] --in short, I > think there's multiple things generating code coverage stats for the same > JSScript and they're not getting properly coalesced. So far only the interpreter and Baseline are supposed to increment the same counters, and even if IonMonkey does not increment these counters at the moment, it will never inline a function which never got executed before in baseline. Thus, with the current system, any function should at least run once in the interpreter, and collect coverage information about the entry of the function. So if there is no coverage information, I wonder if this might mean that we relazified even if we forbid to do any lazy-parsing. In which case this should still be caught by the assertion.

I am able to reproduce this issue on some JS test cases, such as jit-tests/tests/ion/recover-arrays.js where the for-loop has hit for each line, and the function do not have any hit for their body. I will investigate.

Ok, the problem is that some scripts are being GCed before the top-level script which has references to these scripts, which was one of the assumption I made based on the fact that when the lazyScript parsing is disabled all script should be created in order. Apparently this assumption does not hold, thus I would have to make the collection of the code coverage asynchronous.

This patch removes the traversal made by LCovSource::writeTopLevelScript and rely on the GC to do the traversal of all the scripts. Even if this lack the determinism we might have hoped for while generating the lcov file, this should still provide a valid output assuming that all scripts are properly GC-ed before the end of the compartment.

Comment on attachment 8686710 [details] [diff] [review] LCov: Rely on the GC finalizers to visit all JSScripts. Review of attachment 8686710 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/CodeCoverage.cpp @@ +351,5 @@ > + // If this script is the top-level script, then record it such that we can > + // assume that the code coverage report is complete, as this script has > + // references on all inner scripts. > + if (script->isTopLevel()) > + hasTopLevelScript_ = true; What is this test needed for? There can be multiple top level scripts so a compartment can have a top level script as well as partial scripts which parsing/emitting failed for.

(In reply to Brian Hackett (:bhackett) from comment #10) > > + if (script->isTopLevel()) > > + hasTopLevelScript_ = true; > > What is this test needed for? There can be multiple top level scripts so a > compartment can have a top level script as well as partial scripts which > parsing/emitting failed for. This is needed to ensure that we have the entire content of the file, so that it did not failed the full-parse which is used when LCov reports are enabled. We can have multiple top-level for one compartment, but at most one top-level for each Source. isTopLevel check the "code" variable to ensure that parsing did not failed.

Comment on attachment 8686710 [details] [diff] [review] LCov: Rely on the GC finalizers to visit all JSScripts. Review of attachment 8686710 [details] [diff] [review]: ----------------------------------------------------------------- OK, thanks, I missed that LCovSource is per-source.

I will merge this into the previous patch to let the test suite pass again. This patch only expose the disableLazyParsing flag to the newGlobal function of the JS shell, and use it inside coverage/simple.js . This patch makes the test closer to the use of the environment variable used for triggering the LCov output.