Closed Bug 1597997 Opened 2 years ago Closed 2 years ago

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


(Core :: JavaScript Engine, defect, P2)




Tracking Status
firefox-esr68 --- unaffected
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- fixed


(Reporter: nchevobbe, Assigned: tcampbell)


(Blocks 1 open bug, Regression)


(Keywords: regression)


(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)

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:

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

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.

Priority: P3 → P2
Resolution: WORKSFORME → ---
Attached file
I've analyzed the coverage artifacts for 0016ade97e65a470bc97521ab8338210e74cdd02.

I looked for a coverage artifact where line was covered, but its encapsulating function wasn't.

I found

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
Initialize inferred function name before script. r=jorendorff
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.