Perhaps due to its size, I can't find any plausible coverage for browser.js after running a directory of browser mochitests (browser/base/content/test/newtab in this case). There are plenty of BRDA, FN, and DA entries in the generated lcov, but everything reports that no functions, branches, or lines were covered by the test run. I thought perhaps this was due to bug 1213735, but the result is the same after pulling and building the fix.

(In reply to Chris Manchester [:chmanchester] from comment #0) > Perhaps due to its size, I can't find any plausible coverage for browser.js > after running a directory of browser mochitests > (browser/base/content/test/newtab in this case). I have no checked yet, but the only expected reason for not reporting any code coverage information is if we are not closing the JSRuntime properly, such as using an exit function to quit Firefox without going through the finalizers of the JSRuntime. How do we close Firefox when after running the mochitests?

(In reply to Nicolas B. Pierron [:nbp] from comment #1) > (In reply to Chris Manchester [:chmanchester] from comment #0) > > Perhaps due to its size, I can't find any plausible coverage for browser.js > > after running a directory of browser mochitests > > (browser/base/content/test/newtab in this case). > > I have no checked yet, but the only expected reason for not reporting any > code coverage information is if we are not closing the JSRuntime properly, > such as using an exit function to quit Firefox without going through the > finalizers of the JSRuntime. > > How do we close Firefox when after running the mochitests? We use nsIAppStartup.quit -- do we need to notify an additional observer or similar to ensure finalizers are run? As far as I can tell, the child process is signalled by the parent when e10s-enabled firefox exits, so we might always have this problem for the child process. Is there be some way we can trigger collection from privileged JS in the mochitest harness? Or, could we add one?

I uploaded http://people.mozilla.org/~cmanchester/jscov/ with the coverage report I'm looking at. The results for content scripts look similar.

(In reply to Chris Manchester (away Dec. 3-7) from comment #2) > As far as I can tell, the child process is signalled by the parent when > e10s-enabled firefox exits, so we might always have this problem for the > child process. Is there be some way we can trigger collection from > privileged JS in the mochitest harness? Or, could we add one? I think we can add a JSAPI function to force a Runtime to output everything. We already have a similar function for testing purpose, so I hope this should not be too hard. I will make a patch to spew everything out of one JSRuntime.

This is still a problem. I've tried running the tests under "browser/base/content/test/chrome/" and the coverage for some of the content scripts (e.g. "browser.js") is 0. You can also reproduce by just launching Firefox. Nicolas, can you look into this?

The compartment name for browser.js: TN:moz_2dnullprincipal_3a_7b2992731f_2d9284_2d47f6_2d94e6_2d77f298eac2cd_7d_2c_20XPConnect_20Compilation_20Compartment For content.js and tab-content.js: TN:moz_2dnullprincipal_3a_7b7ffbc10e_2d0cbc_2d46b1_2db33d_2d7f2ccbdccd81_7d_2c_20XPConnect_20Compilation_20Compartment

another case I see is: https://codecov.io/gh/marco-c/gecko-dev/src/714606a8145636d93b116943d3a65a6a49d2acf8/browser/components/customizableui/content/panelUI.js

Additional observations: 1) the issue is reproducible both with e10s on and off; 2) nbp asked me to see if there were any LCovSources incomplete when the LCovCompartment destructor is called: it looks like there are many. I added a simple destructor looping the sources, please confirm it's correct: > LCovCompartment::~LCovCompartment() > { > if (sources_) { > fprintf(stderr, "\n\nLcovCompartment destructor.\n"); > for (LCovSource& source : *sources_) { > if (!source.isComplete()) { > fprintf(stderr, "\n\nLcovCompartment destructor, found incomplete LCovSource.\n"); > } > } > } > } 3) I logged the addresses of the compartments and the source objects (assuming they can't be moved) in collectCodeCoverageInfo and collectSourceFile. It looks like there are a few compartments for which we call collectSourceFile and not collectCodeCoverageInfo, a lot of compartments for which we call collectCodeCoverageInfo and not collectSourceFile. From a quick look, there are a lot of calls to collectSourceFile with different JSCompartment and same ScriptSourceObject (and they seem to be associated with the files with 0 coverage, while the files with > 0 have different ScriptSourceObjects). In particular, it looks like we are calling collectSourceFile in quick succession with a different JSCompartment and same ScriptSourceObject for all the scripts included by "browser.xul". Given point 3, I'm inclined to think this is the same issue as bug 1227735.

(In reply to Marco Castelluccio [:marco] from comment #8) > 2) nbp asked me to see if there were any LCovSources incomplete when the > LCovCompartment destructor is called: it looks like there are many. I added > a simple destructor looping the sources, please confirm it's correct: > > LCovCompartment::~LCovCompartment() > > { > > if (sources_) { > > fprintf(stderr, "\n\nLcovCompartment destructor.\n"); > > for (LCovSource& source : *sources_) { > > if (!source.isComplete()) { > > fprintf(stderr, "\n\nLcovCompartment destructor, found incomplete LCovSource.\n"); > > } > > } > > } > > } This sounds correct. > 3) I logged the addresses of the compartments and the source objects > (assuming they can't be moved) in collectCodeCoverageInfo and > collectSourceFile. It looks like there are a few compartments for which we > call collectSourceFile and not collectCodeCoverageInfo, a lot of > compartments for which we call collectCodeCoverageInfo and not > collectSourceFile. > From a quick look, there are a lot of calls to collectSourceFile with > different JSCompartment and same ScriptSourceObject (and they seem to be I noticed that we tend to reuse the ScriptSourceObject addresses, which might cause false positive later on, as they got garbage collected and re-used. Bug 1227735 should avoid that by removing the ScriptSourceOBject pointer which is used to associate data between JSScript and ScriptSource. > associated with the files with 0 coverage, while the files with > 0 have > different ScriptSourceObjects). In particular, it looks like we are calling > collectSourceFile in quick succession with a different JSCompartment and > same ScriptSourceObject for all the scripts included by "browser.xul". > > Given point 3, I'm inclined to think this is the same issue as bug 1227735. I am not sure that this is exactly the same issue, but it would definitely help reducing the number of false positive reports. Or the issue with weird code coverage of xbl files.

Sorry, disregard point 3 above, I was reading the compartment address and SSO address in the opposite direction. For the "browser.js" SSO object, I can see both collectSourceFile and collectCodeCoverageInfo being called, so the problem must lie somewhere else.

I think I found the problem. For "chrome://browser/content/browser.js" (which is one of the files which shows 0 code coverage): - collectSourceFile is called once with a certain SSO and compartment. - collectCodeCoverageInfo is called several times with hits == 0 with the same SSO and compartment as collectSourceFile. - collectCodeCoverageInfo is called several times with hits != 0 with a different SSO and a different compartment from the previous ones. At the end, the LcovSource associated to the collectCodeCoverageInfo calls with hits != 0 is incomplete because the filename is missing. A possible solution would be to move the collectSourceFile to the JSScript finalizer and store the filename in the JSScript (the SSO is finalized earlier than the JSScript, so we can't get the filename from the SSO in the JSScript finalizer). This is feasible if there's a mapping 1:1 between JSScript and SSO. Other ideas?

We discuss this issue in person, the proposed solution would be to store a map of JSScripts pointers to the to the unique id of their script source object, when the lcov-coverage is enabled. This unique id would then be used as a Key in the LCovSource, to identify which Script Source Object correspond to which JSScript.

I used a slightly different approach. Since the only thing we need from the SSO is the filename, we can use a JSScript->Name map instead of the indirection JSScript->SSO_UUID. This has the added benefit that we don't need to do anything in the SSO finalizer. The UUID approach also presented another problem: some JSScripts are not in the same compartment as their associated SSOs. So the LcovSource associated to these JSScript were still incomplete (missing the filename).

The patch is working for me (e.g. browser.js no longer has 0 coverage). I still need to add comments (and fix some outdated comments) and I think I can also remove the hasScriptUUID_ property from JSScript (I will use the presence in the JSScript->Name map as an indication of hasScriptUUID_; this will mean removing some assertions, but I think it's worth it to avoid the overhead).

Comment on attachment 8882386 [details] [diff] [review] Patch Review of attachment 8882386 [details] [diff] [review]: ----------------------------------------------------------------- This sounds good to me, you might want sfink/jonco feedback as well. ::: js/src/jscompartment.cpp @@ +1345,5 @@ > + // Clear all hasScriptName_ flags of JSScript, in order to release all > + // names of the current compartment. > + for (ScriptNameMap::Range r = scriptNameMap->all(); !r.empty(); r.popFront()) { > + const char* value = r.front().value(); > + r.front().key()->takeOverScriptNameMapEntry(value); From what I understand the script might be finalized at this time, thus checking a boolean value from the JSScript might cause trouble.

Comment on attachment 8882386 [details] [diff] [review] Patch Review of attachment 8882386 [details] [diff] [review]: ----------------------------------------------------------------- Overall this seems like a much simpler approach, which is great. ::: js/src/jscompartment.cpp @@ +1345,5 @@ > + // Clear all hasScriptName_ flags of JSScript, in order to release all > + // names of the current compartment. > + for (ScriptNameMap::Range r = scriptNameMap->all(); !r.empty(); r.popFront()) { > + const char* value = r.front().value(); > + r.front().key()->takeOverScriptNameMapEntry(value); Yes, the script might be dead but not yet finalized at this point. I'm not entirely sure what to do about that. I guess we don't need to reset the script's hasScriptName flag in that case. You can check if the script is dead with IsAboutToBeFinalized(&script). I don't understand why |takeOverScriptNameMapEntry| is called that rather than resetScriptNameFlag or something. ::: js/src/jsgc.cpp @@ +7270,5 @@ > + if (!map) { > + map = cx->new_<ScriptNameMap>(); > + if (!map) { > + ReportOutOfMemory(cx); > + return; What happens if we can't create this map? In general this method is not allowed to fail. Also, can we check if source->scriptNameMap is present / has any entries and skip this if not? @@ +7285,5 @@ > + > + for (ScriptNameMap::Range r = source->scriptNameMap->all(); !r.empty(); r.popFront()) { > + JSScript* key = r.front().key(); > + const char* value = r.front().value(); > + MOZ_ASSERT(target->scriptNameMap->putNew(key, value)); Is it OK if this fails in release builds? ::: js/src/jsscript.cpp @@ +1157,5 @@ > MOZ_ASSERT(p); > return p; > } > > +static inline ScriptNameMap::Ptr GetScriptNameMapEntry(JSScript* script) nit: |static inline ScriptNameMap::Ptr| should be on previous line according to JS engine coding standard. @@ +2678,5 @@ > script->version = options.version; > MOZ_ASSERT(script->getVersion() == options.version); // assert that no overflow occurred > > script->setSourceObject(sourceObject); > + MOZ_ASSERT(script->initScriptName(cx)); We should probably handle this error. @@ +2697,5 @@ > +{ > + MOZ_ASSERT(!hasScriptName()); > + > + if (!filename()) { > + return true; nit: braces are not required for single line if body in JS engine code. @@ +2719,5 @@ > + compartment()->scriptNameMap = map; > + } > + > + // Register the script name in the compartment's map. > + if (!map->putNew(this, strdup(filename()))) { This only happens if coverage is enabled, right? ::: js/src/jsscript.h @@ +242,5 @@ > SystemAllocPolicy> ScriptCountsMap; > +typedef HashMap<JSScript*, > + const char*, > + DefaultHasher<JSScript*>, > + SystemAllocPolicy> ScriptNameMap; These should use ZoneAllocPolicy, but that's an existing bug. ::: js/src/vm/CodeCoverage.cpp @@ +77,1 @@ > hasTopLevelScript_(false) This needs to be initialised first since the order of fields changed. ::: js/src/vm/CodeCoverage.h @@ +32,2 @@ > > // Whether the given script source object matches this LCovSource. The comment still refers to a script source object. @@ +48,5 @@ > // the runtime code coverage trace file. > void exportInto(GenericPrinter& out) const; > > + // Weak pointer of the Script Source Object used by the current source. > + const char* name_; Comment refers to script source object.

(In reply to Jon Coppeard (:jonco) from comment #16) > ::: js/src/jsgc.cpp > @@ +7270,5 @@ > > + if (!map) { > > + map = cx->new_<ScriptNameMap>(); > > + if (!map) { > > + ReportOutOfMemory(cx); > > + return; > > What happens if we can't create this map? In general this method is not > allowed to fail. Also, can we check if source->scriptNameMap is present / > has any entries and skip this if not? > > @@ +7285,5 @@ > > + > > + for (ScriptNameMap::Range r = source->scriptNameMap->all(); !r.empty(); r.popFront()) { > > + JSScript* key = r.front().key(); > > + const char* value = r.front().value(); > > + MOZ_ASSERT(target->scriptNameMap->putNew(key, value)); > > Is it OK if this fails in release builds? I will execute this only if coverage is enabled, so I guess it should be OK? > > @@ +2678,5 @@ > > script->version = options.version; > > MOZ_ASSERT(script->getVersion() == options.version); // assert that no overflow occurred > > > > script->setSourceObject(sourceObject); > > + MOZ_ASSERT(script->initScriptName(cx)); > > We should probably handle this error. Can we assert here if we run this only when coverage is enabled?

(In reply to Marco Castelluccio [:marco] from comment #17) > (In reply to Jon Coppeard (:jonco) from comment #16) > > ::: js/src/jsgc.cpp > > @@ +7270,5 @@ > > > + if (!map) { > > > + map = cx->new_<ScriptNameMap>(); > > > + if (!map) { > > > + ReportOutOfMemory(cx); > > > + return; > > > > What happens if we can't create this map? In general this method is not > > allowed to fail. Also, can we check if source->scriptNameMap is present / > > has any entries and skip this if not? > > > > @@ +7285,5 @@ > > > + > > > + for (ScriptNameMap::Range r = source->scriptNameMap->all(); !r.empty(); r.popFront()) { > > > + JSScript* key = r.front().key(); > > > + const char* value = r.front().value(); > > > + MOZ_ASSERT(target->scriptNameMap->putNew(key, value)); > > > > Is it OK if this fails in release builds? > > I will execute this only if coverage is enabled, so I guess it should be OK? That would not register the entry in optimized builds? > > > > @@ +2678,5 @@ > > > script->version = options.version; > > > MOZ_ASSERT(script->getVersion() == options.version); // assert that no overflow occurred > > > > > > script->setSourceObject(sourceObject); > > > + MOZ_ASSERT(script->initScriptName(cx)); > > > > We should probably handle this error. > > Can we assert here if we run this only when coverage is enabled? No, fuzzers are everywhere and they would find a way … Also, this would not be initialized in optimizied builds. I would prefer to keep it possible to run code coverage in optimized builds even if this is not possible in Gecko.

(In reply to Nicolas B. Pierron [:nbp] from comment #18) > (In reply to Marco Castelluccio [:marco] from comment #17) > > (In reply to Jon Coppeard (:jonco) from comment #16) > > > ::: js/src/jsgc.cpp > > > @@ +7270,5 @@ > > > > + if (!map) { > > > > + map = cx->new_<ScriptNameMap>(); > > > > + if (!map) { > > > > + ReportOutOfMemory(cx); > > > > + return; > > > > > > What happens if we can't create this map? In general this method is not > > > allowed to fail. Also, can we check if source->scriptNameMap is present / > > > has any entries and skip this if not? > > > > > > @@ +7285,5 @@ > > > > + > > > > + for (ScriptNameMap::Range r = source->scriptNameMap->all(); !r.empty(); r.popFront()) { > > > > + JSScript* key = r.front().key(); > > > > + const char* value = r.front().value(); > > > > + MOZ_ASSERT(target->scriptNameMap->putNew(key, value)); > > > > > > Is it OK if this fails in release builds? > > > > I will execute this only if coverage is enabled, so I guess it should be OK? > > That would not register the entry in optimized builds? > > > > > > > @@ +2678,5 @@ > > > > script->version = options.version; > > > > MOZ_ASSERT(script->getVersion() == options.version); // assert that no overflow occurred > > > > > > > > script->setSourceObject(sourceObject); > > > > + MOZ_ASSERT(script->initScriptName(cx)); > > > > > > We should probably handle this error. > > > > Can we assert here if we run this only when coverage is enabled? > > No, fuzzers are everywhere and they would find a way … > Also, this would not be initialized in optimizied builds. > > I would prefer to keep it possible to run code coverage in optimized builds > even if this is not possible in Gecko. I actually meant MOZ_RELEASE_ASSERT. If it's not feasible I will handle the error somehow.

One thing still missing is fixing the jit-test/tests/coverage/simple.js test (which is the only one failing in a try build).

Comment on attachment 8882588 [details] [diff] [review] Patch Review of attachment 8882588 [details] [diff] [review]: ----------------------------------------------------------------- I'll finish the review later. Here are some preliminary remarks: ::: js/src/jsgc.cpp @@ +7275,5 @@ > + > + for (ScriptNameMap::Range r = source->scriptNameMap->all(); !r.empty(); r.popFront()) { > + JSScript* key = r.front().key(); > + const char* value = r.front().value(); > + MOZ_RELEASE_ASSERT(target->scriptNameMap->putNew(key, value)); use AutoEnterOOMUnsafeRegion. http://searchfox.org/mozilla-central/source/js/src/vm/ArrayBufferObject.cpp#348,350 ::: js/src/jsscript.cpp @@ +1160,5 @@ > > +static inline ScriptNameMap::Ptr > +GetScriptNameMapEntry(JSScript* script) > +{ > + MOZ_ASSERT(script->hasScriptName()); nit: remove this assertion as this is the same as the other one below. @@ +2677,5 @@ > MOZ_ASSERT(script->getVersion() == options.version); // assert that no overflow occurred > > script->setSourceObject(sourceObject); > + if (cx->runtime()->lcovOutput().isEnabled()) > + MOZ_RELEASE_ASSERT(script->initScriptName(cx)); use AutoEnterOOMUnsafeRegion or return nullptr.

Comment on attachment 8882588 [details] [diff] [review] Patch Review of attachment 8882588 [details] [diff] [review]: ----------------------------------------------------------------- This is not the most optimized way of doing it, if this is fine by your standard of running CCov builds, I am fine with this patch. ::: js/src/jsscript.cpp @@ +3138,5 @@ > > // Collect code coverage information for this script and all its inner > // scripts, and store the aggregated information on the compartment. > + if (fop->runtime()->lcovOutput().isEnabled() && hasScriptName()) { > + compartment()->lcovOutput.collectCodeCoverageInfo(compartment(), this); If we only report the JSScript which have "hasScriptName" we would never catch issues like the zero-count issues, right? Shouldn't the fact of not having a script name be an assertion instead? ::: js/src/vm/CodeCoverage.h @@ +32,4 @@ > > + // Whether the given script name matches this LCovSource. > + bool match(const char* name) const { > + return strcmp(name_, name) == 0; I would definitely prefer a unique id if possible: 1. This would avoid name clashes, if the same source is loaded twice in the same compartment. 2. This would be faster to compare.

(In reply to Nicolas B. Pierron [:nbp] from comment #23) > Comment on attachment 8882588 [details] [diff] [review] > Patch > > Review of attachment 8882588 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is not the most optimized way of doing it, if this is fine by your > standard of running CCov builds, I am fine with this patch. I think it should be fine. If we find it too slow, we can optimize it later on. > ::: js/src/jsscript.cpp > @@ +3138,5 @@ > > > > // Collect code coverage information for this script and all its inner > > // scripts, and store the aggregated information on the compartment. > > + if (fop->runtime()->lcovOutput().isEnabled() && hasScriptName()) { > > + compartment()->lcovOutput.collectCodeCoverageInfo(compartment(), this); > > If we only report the JSScript which have "hasScriptName" we would never > catch issues like the zero-count issues, right? > Shouldn't the fact of not having a script name be an assertion instead? There are some scripts that have no filename and some scripts with filename "self-hosted" that are removed from the map before the finalizer is called (the finishRoots() -> clearScriptNames() is called before the JSScript finalizer). > ::: js/src/vm/CodeCoverage.h > @@ +32,4 @@ > > > > + // Whether the given script name matches this LCovSource. > > + bool match(const char* name) const { > > + return strcmp(name_, name) == 0; > > I would definitely prefer a unique id if possible: > 1. This would avoid name clashes, if the same source is loaded twice in the > same compartment. > 2. This would be faster to compare. 1 should not be a problem, right? As we are interested in the overall coverage, we would be collapsing the equal SF entries into one anyway. 2 yes, but the UUID solution seems more complex and doesn't work with a simple map, as there are some compartments where the function to collect the source file name (called from the SSO finalizer) is not called and so the function to collect coverage info (called from the JSScript finalizer) cannot find the corresponding LcovSource entry with the same UUID in the LcovCompartment's vector of LcovSource entries. Probably this is due to the fact that in some compartments we have a wrapper of the SSO. If we find the suboptimal solution to be a problem, we can always think about how to optimize it by using a UUID in a follow-up.

Comment on attachment 8882588 [details] [diff] [review] Patch Review of attachment 8882588 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. r=me providing test failures are fixed.

I've fixed the failing tests and addressed the review comments. Carrying r+.

Pushed by mcastelluccio@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d35a0173ec7c Introduce a map to match JSScript to script filenames instead of relying on ScriptSourceObject. r=nbp,jonco

Comment on attachment 8883002 [details] [diff] [review] Patch Review of attachment 8883002 [details] [diff] [review]: ----------------------------------------------------------------- I think you should duplicate the string before storing it on the LCovSource, and delete the new allocated strings when you delete entries from the hash map, and also when the LCovSource structure is deleted. ::: js/src/jsscript.cpp @@ +1366,5 @@ > void > +JSScript::destroyScriptName() > +{ > + auto p = GetScriptNameMapEntry(this); > + compartment()->scriptNameMap->remove(p); 4. Leaked, while it should be freed (*). @@ +2732,5 @@ > + > + compartment()->scriptNameMap = map; > + } > + > + char* name = js_strdup(filename()); 0. New string allocated. @@ +2739,5 @@ > + return false; > + } > + > + // Register the script name in the compartment's map. > + if (!map->putNew(this, name)) { 1. Saved as value of one HashMap entry. ::: js/src/vm/CodeCoverage.cpp @@ +436,5 @@ > } > } > > // Allocate a new LCovSource for the current top-level. > + if (!sources_->append(Move(LCovSource(&alloc_, name)))) { 3. Stored on the LCovSource without making a copy. ::: js/src/vm/CodeCoverage.h @@ +32,4 @@ > > + // Whether the given script name matches this LCovSource. > + bool match(const char* name) const { > + return strcmp(name_, name) == 0; 5*. Use-after-free.

Pushed by mcastelluccio@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/843748765dc8 Avoid possible leak of script names from the JSScript -> script name map. r=nbp

https://hg.mozilla.org/mozilla-central/rev/d35a0173ec7c https://hg.mozilla.org/mozilla-central/rev/843748765dc8

backed this out after talking to marco and nbp on irc for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=111668767&repo=mozilla-central

Backout by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/7620030e2a33 Backed out changeset 843748765dc8 https://hg.mozilla.org/mozilla-central/rev/5bed7af56951 Backed out changeset d35a0173ec7c for unexpected test failures

This patch contains both the first and second changeset that landed and were backed out plus the fix for bug 1378068, which turned out to be pretty simple. The linux64-ccov failures are still unresolved, they are caused by a double free coming from clearScriptNames. I'm not sure yet how that is happening, the string allocated in initScriptName is only freed in destroyScriptName or clearScriptNames. When it is freed in destroyScriptName (called from JSScript::finalize), the element is removed from the map, so it shouldn't be freed again in clearScriptNames.

I've figured out where the double frees are coming from (the off-thread compartment). I will upload a new patch shortly.

The changes compared to the first version of the patch are minimal. I will post two interdiffs.

Comment on attachment 8883292 [details] [diff] [review] Patch Review of attachment 8883292 [details] [diff] [review]: ----------------------------------------------------------------- Interdiffs look good to me.

Comment on attachment 8883292 [details] [diff] [review] Patch Review of attachment 8883292 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsopcode.cpp @@ +2976,5 @@ > RootedScript script(cx); > RootedFunction fun(cx); > do { > script = queue.popCopy(); > + if (cx->runtime()->lcovOutput().isEnabled() && !script->initScriptName(cx)) initScriptName asserts that hasScriptName is false, which I would not expect to be the case in jit-test/tests/coverage/simple.js Also, this assert will trigger is getLcovInfo is called twice. cx->runtime()->lcovOutput().isEnabled() is unlikely to change during this function, move this condition higher and do not return false, unless you call JS_ReportErrorASCII. ::: js/src/vm/CodeCoverage.h @@ +28,5 @@ > class LCovSource > { > public: > + explicit LCovSource(LifoAlloc* alloc, const char* name); > + LCovSource(LCovSource&& src); nit: Add "explicit" keyword in front on single argument constructors.

Comment on attachment 8883292 [details] [diff] [review] Patch Review of attachment 8883292 [details] [diff] [review]: ----------------------------------------------------------------- (resetting jonco r+, as I mess-up with the flags while submitting my review)

I'm creating and adding the script name in GenerateLcovInfo if the script doesn't already have a script name. I'm destroying the script name in GenerateLcovInfo if it was created in GenerateLcovInfo itself (to avoid issues with dead JSScripts in the map). The issue is that GenerateLcovInfo must work also when the JS_CODE_COVERAGE_OUTPUT_DIR environment variable is not set, and currently when this is not set we do not call destroyScriptName in the JSScript finalizer (as the presence of this environment variable is what enables the code coverage reporting), this is why we need to destroy the script name in GenerateLcovInfo itself.

Comment on attachment 8883320 [details] [diff] [review] Patch Review of attachment 8883320 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsopcode.cpp @@ +2981,5 @@ > + if (!script->hasScriptName()) { > + createdScriptName = true; > + if (!script->initScriptName(cx)) { > + return false; > + } nit: do not brace single lines. @@ +2989,5 @@ > + > + // Destroy the script name if we have created it in this function. > + if (createdScriptName) { > + script->destroyScriptName(); > + } nit: same here. ::: js/src/vm/CodeCoverage.h @@ +27,5 @@ > > class LCovSource > { > public: > + explicit LCovSource(LifoAlloc* alloc, const char* name); nit: remove the explicit keyword. (2 arguments) @@ +28,5 @@ > class LCovSource > { > public: > + explicit LCovSource(LifoAlloc* alloc, const char* name); > + LCovSource(LCovSource&& src); nit: add explicit keyword. (1 argument)

Carrying r+. (In reply to Nicolas B. Pierron [:nbp] from comment #42) > @@ +28,5 @@ > > class LCovSource > > { > > public: > > + explicit LCovSource(LifoAlloc* alloc, const char* name); > > + LCovSource(LCovSource&& src); > > nit: add explicit keyword. (1 argument) I've fixed the other nits, this one can't be done because we have a checker that disallows using explicit with a move constructor.

Pushed by mcastelluccio@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/591b89fa611a Introduce a map to match JSScript to script filenames instead of relying on ScriptSourceObject. r=nbp,jonco