[fuchsia][flatland] Remove always-false debug assertions [chromium/src : main]
Zijie He (Gerrit)
Zijie He voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zijie He
| Auto-Submit | +1 |
1 comment
It's known that the sequence-checker fails on the object. So this CL removes it in favor of unblocking the enablement of DCHECK in the fyi builders.Zijie HeTo be clear, this CL removes an always-failing debug assertion, which was never caught because we don't run dchecks on the builders?
Yes, enabling the dcheck would stop the bleeding.
Related details
- David Song
- Wez
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wez (Gerrit)
Wez added 2 comments![]( "Open in Gerrit")
// TODO(crbug.com/436930319): It's known that this class needs to be thread-safe
// based on the current gfx implementation. It's still unclear if it's expected
// to be thread-safe or the callers are not correctly using it.
// See https://crbug.com/436930319 and https://crbug.com/436929831 for two
// examples of issues.The issue is that the sequence-checker makes this class thread-_hostile_ (i.e. incapable of being used multi-threaded), whereas the `gfx` expectation appears to be that it is thread-_unsafe_ (i.e. is not intrinsically thread-safe, but can be used multi-threaded if the caller synchronizes access).
SEQUENCE_CHECKER(sequence_checker_);As mentioned offline, ideally we'd leave a sequence checker in place and apply it to create/destroy, then have a separate one for use, to enforce those two thread-affine properties - or whatever arrangement reflects the actual undocumented expectation on implementations.
Related details
- David Song
- Zijie He
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zijie He
| Auto-Submit | +1 |
2 comments
// TODO(crbug.com/436930319): It's known that this class needs to be thread-safe
// based on the current gfx implementation. It's still unclear if it's expected
// to be thread-safe or the callers are not correctly using it.
// See https://crbug.com/436930319 and https://crbug.com/436929831 for two
// examples of issues.The issue is that the sequence-checker makes this class thread-_hostile_ (i.e. incapable of being used multi-threaded), whereas the `gfx` expectation appears to be that it is thread-_unsafe_ (i.e. is not intrinsically thread-safe, but can be used multi-threaded if the caller synchronizes access).
I doubt gfx is the only use of this class, indeed the two known failures come from cc/ but not gfx/. Not sure about the relationship between gfx and cc, but none of them use lock or indicate the thread-affinity immediately.
Updated the comment here to make it vague and avoid the impression that thread-safety is required.
SEQUENCE_CHECKER(sequence_checker_);As mentioned offline, ideally we'd leave a sequence checker in place and apply it to create/destroy, then have a separate one for use, to enforce those two thread-affine properties - or whatever arrangement reflects the actual undocumented expectation on implementations.
As mentioned in https://crrev.com/c/6909839, the destructor calls unmap conditionally, using two sequence-checkers. It's the reason I am making a much simpler workaround here and will continue the discussion in https://crrev.com/c/6909839.
The sequence checker is failing and does not provide its expected functionality other than blocking the enablement of the DCHECK and causes potentially other similar DCHECK failures to slip through.
Related details
- David Song
- Wez
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Song (Gerrit)
David Song voted Code-Review+1![]( "Open in Gerrit")
- Wez
- Zijie He
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zijie He
| Commit-Queue | +2 |
1 comment
Thank you David.
Wez, let's move the proper fix back to https://crrev.com/c/6909839. I replied the latest comment, and would continue the investigation of the guesses we had.
Related details
- Wez
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[fuchsia][flatland] Remove always-false debug assertions
These sequence check assertions always fail, but went unnoticed because
our builders do not run with dcheck_always_on=true (being addressed in
crbug.com/436929831). Remove them and replace with TODOs to figure out
the correct assertions in https://crrev.com/c/6909839.
- M ui/ozone/platform/flatland/client_native_pixmap_factory_flatland.cc
Code-Review: +1 by David Song
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |