Remove dangling pointers in mock_surface [chromium/src : main]
AbdAlRahman Gad (Gerrit)
AbdAlRahman Gad added 1 comment![]( "Open in Gerrit")
I was trying to do something like this in the [Attach function](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/test/mock_surface.cc;l=15) but it didn't work well:
```
void Attach(wl_client* client,
wl_resource* resource,
wl_resource* buffer_resource,
int32_t x,
int32_t y) {
auto* surface = GetUserDataAs<MockSurface>(resource);
std::vector<base::ScopedFD> empty_fds;
wl_resource* b_resource =
CreateResourceWithImpl<::testing::NiceMock<TestBuffer>>(
client, &wl_buffer_interface, 1, &kTestWlBufferImpl, 0,
base::BindOnce(&MockSurface::ResetBuffer, surface->GetWeakPtr(),
buffer_resource),
std::move(empty_fds));
surface->AttachNewBuffer(b_resource, x, y);
}
```
I think I need help on how to fix these dangling pointers.
Related details
- Max Ihlenfeldt
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Max Ihlenfeldt (Gerrit)
Max Ihlenfeldt added 1 comment![]( "Open in Gerrit")
I was trying to do something like this in the [Attach function](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/test/mock_surface.cc;l=15) but it didn't work well:
```
void Attach(wl_client* client,
wl_resource* resource,
wl_resource* buffer_resource,
int32_t x,
int32_t y) {
auto* surface = GetUserDataAs<MockSurface>(resource);std::vector<base::ScopedFD> empty_fds;
wl_resource* b_resource =
CreateResourceWithImpl<::testing::NiceMock<TestBuffer>>(
client, &wl_buffer_interface, 1, &kTestWlBufferImpl, 0,
base::BindOnce(&MockSurface::ResetBuffer, surface->GetWeakPtr(),
buffer_resource),
std::move(empty_fds));surface->AttachNewBuffer(b_resource, x, y);
}
```I think I need help on how to fix these dangling pointers.
I think a log of running a test that fails due to the dangling pointer with `WAYLAND_DEBUG=client` might help here, could you please gather one and post it (or a link to it, depending on how long it is) here?
Related details
- AbdAlRahman Gad
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AbdAlRahman Gad (Gerrit)
AbdAlRahman Gad added 1 comment![]( "Open in Gerrit")
Max IhlenfeldtI was trying to do something like this in the [Attach function](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/test/mock_surface.cc;l=15) but it didn't work well:
```
void Attach(wl_client* client,
wl_resource* resource,
wl_resource* buffer_resource,
int32_t x,
int32_t y) {
auto* surface = GetUserDataAs<MockSurface>(resource);std::vector<base::ScopedFD> empty_fds;
wl_resource* b_resource =
CreateResourceWithImpl<::testing::NiceMock<TestBuffer>>(
client, &wl_buffer_interface, 1, &kTestWlBufferImpl, 0,
base::BindOnce(&MockSurface::ResetBuffer, surface->GetWeakPtr(),
buffer_resource),
std::move(empty_fds));surface->AttachNewBuffer(b_resource, x, y);
}
```I think I need help on how to fix these dangling pointers.
I think a log of running a test that fails due to the dangling pointer with `WAYLAND_DEBUG=client` might help here, could you please gather one and post it (or a link to it, depending on how long it is) here?
- Max Ihlenfeldt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Max Ihlenfeldt (Gerrit)
Max Ihlenfeldt added 1 comment![]( "Open in Gerrit")
Max IhlenfeldtI was trying to do something like this in the [Attach function](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/test/mock_surface.cc;l=15) but it didn't work well:
```
void Attach(wl_client* client,
wl_resource* resource,
wl_resource* buffer_resource,
int32_t x,
int32_t y) {
auto* surface = GetUserDataAs<MockSurface>(resource);std::vector<base::ScopedFD> empty_fds;
wl_resource* b_resource =
CreateResourceWithImpl<::testing::NiceMock<TestBuffer>>(
client, &wl_buffer_interface, 1, &kTestWlBufferImpl, 0,
base::BindOnce(&MockSurface::ResetBuffer, surface->GetWeakPtr(),
buffer_resource),
std::move(empty_fds));surface->AttachNewBuffer(b_resource, x, y);
}
```I think I need help on how to fix these dangling pointers.
AbdAlRahman GadI think a log of running a test that fails due to the dangling pointer with `WAYLAND_DEBUG=client` might help here, could you please gather one and post it (or a link to it, depending on how long it is) here?
Hm, I think this might be non-trivial to do correctly without dangling pointers. Maybe we can use weak pointers, similar to https://crrev.com/c/6787208. Kramer, wdyt?
Related details
- AbdAlRahman Gad
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
Yes you need to store the weakptr to test_buffers in `AttachNewBuffer` and get `wl_resource` from it indirectly when the these getters are called.
Related details
- AbdAlRahman Gad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AbdAlRahman Gad (Gerrit)
AbdAlRahman Gad added 1 comment![]( "Open in Gerrit")
attached_buffer_ = buffer_resource;Could you please me with how to assign `wl_resource* buffer_resource` to `base::WeakPtr<wl_resource> attached_buffer_`?
Also, is my approach correct? I was going to use `base::WeakPtr<WaylandBufferHandle>` but it's using `wl_buffer` instead of `wl_resource` so I thought it's not the right thing to do.
Related details
- Kramer Ge
- Max Ihlenfeldt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
attached_buffer_ = buffer_resource;Could you please me with how to assign `wl_resource* buffer_resource` to `base::WeakPtr<wl_resource> attached_buffer_`?
Also, is my approach correct? I was going to use `base::WeakPtr<WaylandBufferHandle>` but it's using `wl_buffer` instead of `wl_resource` so I thought it's not the right thing to do.
You can use [TestBuffer](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/test/test_buffer.h;l=21;bpv=0;bpt=1), it's a `ServerObject`, whose lifecycle is controlled by the `wl_resource` in wayland if it's created from `CreateResourceWithImpl` [here](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/test/test_zwp_linux_buffer_params.cc;l=40;drc=391e98eeaee80326c3a9d9e9dd731570dba0981a;bpv=0;bpt=1). You can add a `WeakPtrFactory` to the `TestBuffer`, and the `MockSurface` would store a `weak_ptr` of the test_buffer and invoke `resource()` to get the `wl_resource`. To get `TestBuffer` from `resource`, you call `GetUserDataAs<TestBuffer>()`
Related details
- AbdAlRahman Gad
- Max Ihlenfeldt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AbdAlRahman Gad (Gerrit)
AbdAlRahman Gad added 1 comment![]( "Open in Gerrit")
attached_buffer_ = GetUserDataAs<TestBuffer>(buffer_resource)->GetWeakPtr();Calling `GetWeakPtr()` fails here.
A failed test example:
```
[ RUN ] WaylandFrameManagerTest.UnblockFrames_OnTransitionToSwapWithoutCommit
[2296185:2296185:WARNING:ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc:444] Failed to initialize drm render node handle.
[2296185:2296185:WARNING:ui/ozone/platform/wayland/common/wayland_object.cc:119] Binding to wl_shm version 1 but version 2 is available.
[2296185:2296185:WARNING:ui/ozone/platform/wayland/common/wayland_object.cc:119] Binding to zwp_pointer_gestures_v1 version 1 but version 3 is available.
[2296185:2296185:WARNING:ui/ozone/platform/wayland/host/wayland_surface.cc:200] Server doesn't support wp_content_type_v1
[2296185:2296185:WARNING:ui/ozone/platform/wayland/host/wayland_surface.cc:214] Server doesn't support wp_color_management_surface_v1.
Received signal 11 SI_KERNEL000000000000
Possibly a General Protection Fault, can be due to a non-canonical address dereference. See "Intel 64 and IA-32 Architectures Software Developer’s Manual", Volume 1, Section 3.3.7.1.
#0 0x639ed07af869 base::debug::CollectStackTrace() [../../base/debug/stack_trace_posix.cc:1052:7]
#1 0x639ed077e19a base::debug::StackTrace::StackTrace() [../../base/debug/stack_trace.cc:255:20]
#2 0x639ed077e105 base::debug::StackTrace::StackTrace() [../../base/debug/stack_trace.cc:250:28]
#3 0x639ed07af0d9 base::debug::(anonymous namespace)::StackDumpSignalHandler() [../../base/debug/stack_trace_posix.cc:487:3]
#4 0x73d36fa45330 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x4532f)
#5 0x639ecdc7bd1b std::__Cr::__cxx_atomic_load<>() [gen/third_party/libc++/src/include/__atomic/support/c11.h:81:10]
#6 0x639ecdc7bccb std::__Cr::__atomic_base<>::load() [gen/third_party/libc++/src/include/__atomic/atomic.h:70:12]
#7 0x639ed051bbca base::AtomicRefCount::IsOne() [../../base/atomic_ref_count.h:52:42]
#8 0x639ed051b8f5 base::subtle::RefCountedThreadSafeBase::HasOneRef() [../../base/memory/ref_counted.cc:25:21]
#9 0x639ed05207fd base::internal::WeakReferenceOwner::HasRefs() [../../base/memory/weak_ptr.h:163:41]
#10 0x639ed05200c0 base::internal::WeakReferenceOwner::GetRef() [../../base/memory/weak_ptr.cc:95:8]
#11 0x639ecf5ce3b9 _ZN4base14WeakPtrFactoryIN2wl10TestBufferEE10GetWeakPtrEvQntsr3stdE10is_const_vIT_E [../../base/memory/weak_ptr.h:382:45]
#12 0x639ecf5ce269 wl::TestBuffer::GetWeakPtr() [../../ui/ozone/platform/wayland/test/test_buffer.cc:17:28]
#13 0x639ecf592952 wl::MockSurface::AttachNewBuffer() [../../ui/ozone/platform/wayland/test/mock_surface.cc:157:68]
#14 0x639ecf591fbb wl::(anonymous namespace)::Attach() [../../ui/ozone/platform/wayland/test/mock_surface.cc:22:12]
#15 0x639ed38f0405 ffi_call_unix64
#16 0x639ed38ef959 ffi_call_int
#17 0x639ecf530497 wl_closure_invoke [../../third_party/wayland/src/src/connection.c:1228:2]
#18 0x639ecf5fa656 wl_client_connection_data [../../third_party/wayland/src/src/wayland-server.c:444:4]
#19 0x639ecf5f7e1e wl_event_source_fd_dispatch [../../third_party/wayland/src/src/event-loop.c:113:9]
#20 0x639ecf5f94be wl_event_loop_dispatch [../../third_party/wayland/src/src/event-loop.c:1105:4]
#21 0x639ecf5e957a wl::TestWaylandServerThread::OnFileCanReadWithoutBlocking() [../../ui/ozone/platform/wayland/test/test_wayland_server_thread.cc:299:3]
#22 0x639ed07d1124 base::MessagePumpEpoll::FdWatchController::OnFdReadable() [../../base/message_loop/message_pump_epoll.cc:761:13]
#23 0x639ed07d0e8e base::MessagePumpEpoll::HandleEvent() [../../base/message_loop/message_pump_epoll.cc:669:17]
#24 0x639ed07d09f7 base::MessagePumpEpoll::OnEpollEvent() [../../base/message_loop/message_pump_epoll.cc:615:7]
#25 0x639ed07cf883 base::MessagePumpEpoll::WaitForEpollEvents() [../../base/message_loop/message_pump_epoll.cc:506:7]
#26 0x639ed07cf172 base::MessagePumpEpoll::Run() [../../base/message_loop/message_pump_epoll.cc:285:5]
#27 0x639ed0668837 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run() [../../base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:647:12]
#28 0x639ed058a1fb base::RunLoop::Run() [../../base/run_loop.cc:134:14]
#29 0x639ed0713049 base::Thread::Run() [../../base/threading/thread.cc:361:13]
#30 0x639ed0713482 base::Thread::ThreadMain() [../../base/threading/thread.cc:436:3]
#31 0x639ed0769419 base::(anonymous namespace)::ThreadFunc() [../../base/threading/platform_thread_posix.cc:101:13]
#32 0x73d36fa9caa4 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x9caa3)
#33 0x73d36fb29c3c (/usr/lib/x86_64-linux-gnu/libc.so.6+0x129c3b)
r8: 0000000000000000 r9: 000013b400051f00 r10: 0000000000000000 r11: 0000639ecf591f80
r12: 0000000000000000 r13: 000073d36c9f8b20 r14: 000073d36c9f8c68 r15: 000073d36c9f8b1c
di: 0dd0fecaefbeadde si: 0000000000000002 bp: 000073d36c9f88b0 bx: 000073d36c9f8cf0
dx: 0000000000000000 ax: 0dd0fecaefbeadde cx: 0000000000000000 sp: 000073d36c9f88b0
ip: 0000639ecdc7bd1b efl: 0000000000010297 cgf: 002b000000000033 erf: 0000000000000000
trp: 000000000000000d msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]
```
Is there something I should do in order to have a valid `WeakPtr` here?
Related details
- Kramer Ge
- Max Ihlenfeldt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
I guess it's because `ui/ozone/platform/wayland/test/test_wayland_server_thread.h` doesn't implement `wl_shm_pool`, implement it and have it do similar things as `MockZwpLinuxDmabufV1`. Although I'm not fully certain how we ended up with a non-null `buffer_resource` without implementing `wl_shm_pool`.
Related details
- AbdAlRahman Gad
- Kramer Ge
- Max Ihlenfeldt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |