[Blink] Introduce active iterator tracking for HeapVector [chromium/src : main]
Keishi Hattori (Gerrit)
Keishi Hattori added 1 comment![]( "Open in Gerrit")
Related details
- Kent Tamura
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kent Tamura
| Code-Review | +1 |
1 comment
# Enable active iterator counting in WTF::HeapVector to detect use-after-free.nit: `WTF::` -> `blink::`
Related details
- Keishi Hattori
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 2 comments![]( "Open in Gerrit")
Can you check at least Speedometer3 for regressions? We want to know what the cost here is.
#if BUILDFLAG(ENABLE_HEAP_VECTOR_ACTIVE_ITERATOR_CHECKS)Why only HeapVector? Is this the most pressing issue?
Related details
- Keishi Hattori
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keishi Hattori (Gerrit)
Keishi Hattori added 3 comments![]( "Open in Gerrit")
Can you check at least Speedometer3 for regressions? We want to know what the cost here is.
I already ran it and it is quite heavy so I don't plan to ship it as is.
I just want to try enabling it for one Canary release to get the accurate performance cost with PGO.
https://pinpoint-dot-chromeperf.appspot.com/job/1395deab890000
# Enable active iterator counting in WTF::HeapVector to detect use-after-free.Keishi Hattorinit: `WTF::` -> `blink::`
Done
#if BUILDFLAG(ENABLE_HEAP_VECTOR_ACTIVE_ITERATOR_CHECKS)Why only HeapVector? Is this the most pressing issue?
My plan is to tackle non-HeapVector with BRP ref counting.
https://chromium-review.git.corp.google.com/c/chromium/src/+/7843993
I did see bugs involving HeapVector so I think we should try to cover it.
https://b.corp.google.com/issues/497830330
Related details
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
Keishi HattoriCan you check at least Speedometer3 for regressions? We want to know what the cost here is.
I already ran it and it is quite heavy so I don't plan to ship it as is.
I just want to try enabling it for one Canary release to get the accurate performance cost with PGO.
https://pinpoint-dot-chromeperf.appspot.com/job/1395deab890000
That's indeed very heavy for a mitigationg for a single bug class.
(a) Iterator outlives the vector and the vector is completetly gone.
(b) Iterator invalidates because of a modification that re-allocates the backing.
Are both (a) and (b) the problems, or are we only talking about a single bug class mostly.
Related details
- Keishi Hattori
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keishi Hattori (Gerrit)
Keishi Hattori added 1 comment![]( "Open in Gerrit")
Keishi HattoriCan you check at least Speedometer3 for regressions? We want to know what the cost here is.
Michael LippautzI already ran it and it is quite heavy so I don't plan to ship it as is.
I just want to try enabling it for one Canary release to get the accurate performance cost with PGO.
https://pinpoint-dot-chromeperf.appspot.com/job/1395deab890000
That's indeed very heavy for a mitigationg for a single bug class.
(a) Iterator outlives the vector and the vector is completetly gone.
(b) Iterator invalidates because of a modification that re-allocates the backing.Are both (a) and (b) the problems, or are we only talking about a single bug class mostly.
This protection should mitigate both.
But my main concern is with (b) which we encountered several in Project Fortify.
Related details
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keishi Hattori (Gerrit)
Keishi Hattori added 1 comment![]( "Open in Gerrit")
Keishi HattoriCan you check at least Speedometer3 for regressions? We want to know what the cost here is.
Michael LippautzI already ran it and it is quite heavy so I don't plan to ship it as is.
I just want to try enabling it for one Canary release to get the accurate performance cost with PGO.
https://pinpoint-dot-chromeperf.appspot.com/job/1395deab890000
Keishi HattoriThat's indeed very heavy for a mitigationg for a single bug class.
(a) Iterator outlives the vector and the vector is completetly gone.
(b) Iterator invalidates because of a modification that re-allocates the backing.Are both (a) and (b) the problems, or are we only talking about a single bug class mostly.
This protection should mitigate both.
But my main concern is with (b) which we encountered several in Project Fortify.
Would it be ok to land it like this, behind a flag, to experiment on one canary release?
In addition to the performance data, if we can get crash reports it could help us locate the reentrancy issues.
Keishi Hattori (Gerrit)
Keishi Hattori voted and added 1 comment![]( "Open in Gerrit")
Votes added by Keishi Hattori
| Commit-Queue | +2 |
1 comment
As was discussed in chat, I've added the link to the design doc, and I will land this.
Related details
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keishi Hattori (Gerrit)
Keishi Hattori voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Keishi Hattori (Gerrit)
Keishi Hattori added 1 comment![]( "Open in Gerrit")
I reset the CR+1 bit on this one while rebasing too. Would you take another look.
Related details
- Kent Tamura
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura voted Code-Review+1![]( "Open in Gerrit")
- Keishi Hattori
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keishi Hattori (Gerrit)
Keishi Hattori voted Commit-Queue+2![]( "Open in Gerrit")
- Michael Lippautz
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Blink] Introduce active iterator tracking for HeapVector
Add an active iterator counter to WTF::HeapVector to detect dangling
iterators when a container backing store is reallocated or freed. This
safeguard is controlled by the new experimental build flag
ENABLE_HEAP_VECTOR_ACTIVE_ITERATOR_CHECKS.
To accommodate tracking overhead without breaking binary regression
tests, container size assertions (`ASSERT_SIZE`) are conditionally
bypassed when the flag is enabled. Structural layout verification types
in ShapeResult are updated to match the instrumentation metrics.
Design doc: https://docs.google.com/document/d/1xKOsdXdgnsORfUyA3CBAtUK3tIka50KBFF1lei8P9jA/edit?tab=t.0#heading=h.c0uts5ftkk58
- M third_party/blink/renderer/platform/fonts/shaping/shape_result.cc
- M third_party/blink/renderer/platform/heap/collection_support/heap_deque.h
- M third_party/blink/renderer/platform/heap/collection_support/heap_linked_hash_set.h
- M third_party/blink/renderer/platform/heap/collection_support/heap_vector.h
- M third_party/blink/renderer/platform/wtf/BUILD.gn
- M third_party/blink/renderer/platform/wtf/vector.h
Code-Review: +1 by Kent Tamura
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |