New issue
Advanced search Search tips

Issue 827530 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

Wrong coverage

Reported by bak...@scand.com, Mar 30 2018

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce the problem:
1. Open file index.html from attachment coverage.zip
2. Get coverage for it.

What is the expected behavior?
Else block executed and should be highlighted.
https://i.snag.gy/QAikSt.jpg

What went wrong?
Else block ignores.
https://i.snag.gy/sETdUF.jpg

Did this work before? N/A 

Chrome version: 65.0.3325.181  Channel: stable
OS Version: 10.0
Flash Version:
 
coverage.zip
417 bytes Download

Comment 1 by kozy@chromium.org, Mar 30 2018

Cc: caseq@chromium.org
Owner: jgruber@chromium.org
Status: Assigned (was: Unconfirmed)
Interesting, thanks. Here's the corresponding trace:

                    {
                        "functionName": "UtilEscape",
                        "isBlockCoverage": true,
                        "ranges": [
                            {
                                "count": 1,
                                "endOffset": 120,
                                "startOffset": 28
                            },
                            {
                                "count": 0,
                                "endOffset": 88,
                                "startOffset": 67
                            },
                            {
                                "count": 0,
                                "endOffset": 119,
                                "startOffset": 88
                            }
                        ]
                    }

Cc: yangguo@chromium.org
The problem is that when we attempt to merge nested ranges [0], we incorrectly remove the 'else' range with an execution count of one.

[0] https://cs.chromium.org/chromium/src/v8/src/debug/debug-coverage.cc?l=285&rcl=81094bec9a28e792d5e681fb34599638fbdcfe5a
Fix in flight: https://crrev.com/c/992114
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/e42ce2005d535bcbab8c71d683987b68a31fc66f

commit e42ce2005d535bcbab8c71d683987b68a31fc66f
Author: jgruber <jgruber@chromium.org>
Date: Wed Apr 04 12:46:24 2018

[coverage] Fix invalid coverage block transformation

Before reporting coverage data, we attempt to reduce clutter by
merging nested and consecutive ranges. Nested ranges are merged, if
the child range has the same execution count as the parent range.
Sibling ranges are merged, if one sibling begins where the other ends
and execution counts are identical.

This allowed an invalid transformation in which a range with an
execution count of 1 would be merged into the parent change, but the
sibling range with identical start and end points and a count of 0
would remain, effectively deleting the covered range.

For example:

{start: 0, end: 10, count: 1},
{start: 5, end:  8, count: 1},  // It's invalid to remove this.
{start: 5, end:  8, count: 0}

The fix is to separate the parent and sibling merge passes, and
removing duplicate ranges in-between.

Bug:  chromium:827530 
Change-Id: Ic35eae1d4a106746570ce9cb412ed6710ef6da53
Reviewed-on: https://chromium-review.googlesource.com/992114
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52352}
[modify] https://crrev.com/e42ce2005d535bcbab8c71d683987b68a31fc66f/src/debug/debug-coverage.cc
[modify] https://crrev.com/e42ce2005d535bcbab8c71d683987b68a31fc66f/test/mjsunit/code-coverage-block.js

Labels: Merge-Request-66
Status: Fixed (was: Assigned)
Project Member

Comment 7 by sheriffbot@chromium.org, Apr 4 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Why is this fix needed for m66 vs waiting until 67? 
It's a correctness bug, so ideally we can merge the fix to 66.
how safe is this merge overall? Since this doesn't appear to be release blocking and it's also present in current M65 Stable, my recommendation is to target M67 instead of 66. We're only a week away from Stable, so we're only taking critical changes now. 
In that case let's just wait and not merge at all. The bug is an annoyance, but not critical.
Labels: -Merge-Review-66 Merge-Rejected-66

Comment 13 by bak...@scand.com, May 8 2018

When will it be merged? Do you forget about this bug?)
As per #12 the merge request has been rejected. The fix will be in 67.

Sign in to add a comment