Wrong coverage
Reported by
bak...@scand.com,
Mar 30 2018
|
|||||
Issue descriptionUserAgent: 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:
,
Apr 3 2018
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
}
]
}
,
Apr 3 2018
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
,
Apr 3 2018
Fix in flight: https://crrev.com/c/992114
,
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
,
Apr 4 2018
,
Apr 4 2018
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
,
Apr 5 2018
Why is this fix needed for m66 vs waiting until 67?
,
Apr 5 2018
It's a correctness bug, so ideally we can merge the fix to 66.
,
Apr 9 2018
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.
,
Apr 9 2018
In that case let's just wait and not merge at all. The bug is an annoyance, but not critical.
,
Apr 10 2018
,
May 8 2018
When will it be merged? Do you forget about this bug?)
,
May 8 2018
As per #12 the merge request has been rejected. The fix will be in 67. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by kozy@chromium.org
, Mar 30 2018Owner: jgruber@chromium.org
Status: Assigned (was: Unconfirmed)