Closed Bug 1597997 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla74
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)

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: 2 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
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")
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

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
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

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

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.

Flags: needinfo?(tcampbell)
See Also: → 1624276
You need to log in before you can comment on or make changes to this bug.