Closed Bug 1597997 Opened 5 years ago Closed 5 years ago

Some JSVM coverage reports have functions with wrong names (all "top-level")

Categories

(Core :: JavaScript Engine, defect, P2)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla74
Tracking Flags:
Tracking Status
firefox-esr68 --- unaffected
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- fixed

People

(Reporter: nchevobbe, Assigned: tcampbell)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

jsvm.info
(deleted), text/plain
Details
Bug 1597997 - Initialize inferred function name before script. r?jorendorff
(deleted), text/x-phabricator-request
Details

The priority flag is not set for this bug.
:Sylvestre, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(sledru)

Marco,
Could you please have a look to this and set the right priority?

Flags: needinfo?(sledru) → needinfo?(mcastelluccio)

It looks like it is covered now: https://coverage.moz.tools/#revision=latest&path=devtools%2Fclient%2Fwebconsole%2Fcomponents%2FOutput%2FConsoleTable.js&view=file.

Maybe there was some intermittent issue that caused it not to be covered in that test run.

Status: NEW → RESOLVED
Closed: 5 years ago
Component: Source Code Analysis → Code Coverage
Flags: needinfo?(mcastelluccio)
Priority: -- → P3
Product: Firefox Build System → Testing
Resolution: --- → WORKSFORME

Actually, it is still in the zero coverage report, even though some functions are covered. I'll investigate.

Status: RESOLVED → REOPENED
Priority: P3 → P2
Resolution: WORKSFORME → ---
Attached file jsvm.info (deleted) —
I've analyzed the coverage artifacts for 0016ade97e65a470bc97521ab8338210e74cdd02. I looked for a coverage artifact where line https://hg.mozilla.org/mozilla-central/file/0016ade97e65a470bc97521ab8338210e74cdd02/devtools/client/webconsole/components/Output/ConsoleTable.js#l83 was covered, but its encapsulating function wasn't. I found linux_mochitest-devtools-chrome-16_code-coverage-jsvm.zip. The relevant part of the INFO file is attached. :nbp, any idea what could be causing this?
Component: Code Coverage → JavaScript Engine
Flags: needinfo?(nicolas.b.pierron)
Product: Testing → Core
Summary: [Automated review] Reviewbot wrongly reports some file not being covered by test → Some JSVM coverage reports have functions with wrong names (all "top-level")

This is likely a problem related to this function:
https://searchfox.org/mozilla-central/rev/c61720a7d0c094d772059f9d6a7844eb7619f107/js/src/vm/CodeCoverage.cpp#519-531

Ted can you have a look?

Flags: needinfo?(nicolas.b.pierron) → needinfo?(tcampbell)
Assignee: nobody → tcampbell

This is a regression from Bug 1585372. I had moved the name capturing from finalization to script creation, but functions don't always have their name initialized yet. This means that cases like var x = function() {}; no longer work properly. I'll see what I can do.

Regressed by: 1585372
Depends on: 1606960

The intermittency is due off-thread parse and main-thread parse capturing the name at different times. I'll fix the frontend to be more consistent here.

I also opened Bug 1606960 to close the testing gap between what the jit-tests test and the browser case. Hopefully we can avoid more regressions and improve the reliability of the coverage data.

Flags: needinfo?(tcampbell)

Move the parser's calls to setFunName to before generating the actual script
for the function. This allows code-coverage to save the correct initial
function names when they are inferred. Off-thread parsing defers computing
the coverage metadata so it already has the right result.

Depends on D58668

Blocks: js-code-coverage
Attachment #9118620 - Attachment description: Bug 1597997 - Initialize inferred function name before script. → Bug 1597997 - Initialize inferred function name before script. r?jorendorff
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/11fac2b6352f Initialize inferred function name before script. r=jorendorff

https://hg.mozilla.org/mozilla-central/rev/11fac2b6352f

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
status-firefox74: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

Is this something we should consider uplifting to Beta for Fx73? Please nominate if so.

status-firefox72: --- → wontfix
status-firefox73: --- → affected
status-firefox-esr68: --- → unaffected
Flags: needinfo?(tcampbell)
Flags: in-testsuite+

This is primarily for our own devs so I don't think we should uplift. The patch does have some small risk too.

status-firefox73: affected → wontfix
Flags: needinfo?(tcampbell)
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: