Always post destruction of WebMediaPlayer instances [chromium/src : main]
Dale Curtis (Gerrit)
Dale Curtis voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Daniel Cheng
- Frank Liberato
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato added 1 comment![]( "Open in Gerrit")
for some reason i do not have permission to vote, but lgtm.
-fl
Related details
- Dale Curtis
- Daniel Cheng
- Frank Liberato
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato added 1 comment![]( "Open in Gerrit")
oh i see - it switched to google me and won't let me switch back for some reason. this worked this morning.
-fl
Dale Curtis (Gerrit)
Dale Curtis voted Auto-Submit+1![]( "Open in Gerrit")
- Daniel Cheng
- Frank Liberato
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 8 comments![]( "Open in Gerrit")
I don't love this; there are a lot of other GCed types that are exposed through the Blink public API. I'm not going to say it's painless, but it's not usually that unwieldy either. But I didn't investigate so I don't quite know all the roadblocks that might be involved here...
virtual ~WebMediaPlayer() = default;Probably want to make this protected.
web_media_player_->Shutdown();So code like this can't accidentally use `reset()`. It would mean we would need to pass the task runner to Shutdown() but maybe Shutdown() could post the `delete this` task itself then.
weak_factory_.InvalidateWeakPtrs();```suggestion
weak_factory_.InvalidateWeakPtrsAndDoom();
```
Otherwise people can just call `GetWeakPtr()` again.
// Allow automatic cleanup for tests.Is it hard to fix tests to do the right thing? It would be better not to have this.
weak_factory_.InvalidateWeakPtrs();```suggestion
weak_factory_.InvalidateWeakPtrsAndDoom();
```
void Shutdown() override { weak_ptr_factory_.InvalidateWeakPtrs(); }```suggestion
void Shutdown() override { weak_ptr_factory_.InvalidateWeakPtrsAndDoom(); }
```
Related details
- Dale Curtis
- Frank Liberato
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. |
Frank Liberato (Gerrit)
Frank Liberato voted and added 1 comment![]( "Open in Gerrit")
Votes added by Frank Liberato
| Code-Review | +1 |
1 comment
void Shutdown() override { weak_ptr_factory_.InvalidateWeakPtrs(); }```suggestion
void Shutdown() override { weak_ptr_factory_.InvalidateWeakPtrsAndDoom(); }
```
'...AndDoom'? never heard of that one. it's a cool name, Frodo Baggins.
Related details
- Dale Curtis
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. |
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
I don't love this; there are a lot of other GCed types that are exposed through the Blink public API. I'm not going to say it's painless, but it's not usually that unwieldy either. But I didn't investigate so I don't quite know all the roadblocks that might be involved here...
A quick search doesn't show any GarbageCollected interfaces in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/ ? Can you give an example?
web_media_player_->Shutdown();So code like this can't accidentally use `reset()`. It would mean we would need to pass the task runner to Shutdown() but maybe Shutdown() could post the `delete this` task itself then.
If we don't take your suggestion below, we don't need the complexity you suggest here. Either way is fine, it's just that the singular HTMLME destruction path needs to post. Outside of tests, HTMLME is the only owner of this class.
Can you elaborate on why you think not automatically calling Shutdown() in destruction is better? I can see the appeal with a utility interface with diverse usage, but that's not the case here.
Related details
- Daniel Cheng
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
Dale CurtisI don't love this; there are a lot of other GCed types that are exposed through the Blink public API. I'm not going to say it's painless, but it's not usually that unwieldy either. But I didn't investigate so I don't quite know all the roadblocks that might be involved here...
A quick search doesn't show any GarbageCollected interfaces in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/ ? Can you give an example?
`WebFrame`/`WebLocalFrame` is one example. It's not marked as GCed in the public API [1], since we've decided not to expose Oilpan details outside Blink. But the actual implementation class `WebLocalFrameImpl` is garbage collected [2]; it inherits from `GarbageCollected<T>` and `WebNavigationControl` (the latter of which inherits from `WebLocalFrame`).
Blink's embedder (//content) owns the lifetime of web frames. When the lifetime of the `WebFrame` should end, the embedder calls `WebFrame::Close` [3]. Internally, this releases `WebLocalFrameImpl`'s `SelfKeepAlive` reference–from the perspective of //content, it may no longer use a `WebFrame` once it calls `Close()` on it. This is (part of) the reason `RenderFrameImpl` has `weak_this` checks [4] in various places... (the other reason is missing the `weak_this` check would simply be a use-after-free of the `RenderFrameImpl` :)
It'd be a bit trickier for `WebMediaPlayer`... unlike `WebLocalFrame`, we don't really need the `SelfKeepAlive` since the embedder / //media doesn't hang on to the `WebMediaPlayer` after creating it. The web frame bits have changed significantly over time, but way we originally "solved" it there was by having `WebFrameImplBase` [5]. For `WebMediaPlayer`... I'm actually wondering how much of `WMP` really needs to be exposed in the Blink public API. It seems like media really just needs the factory APIs to create WMP instances, and that we could probably hide all the actual subclass details in Blink... so we could even (potentially) get away with just returning an opaque `WebMediaPlayer` pointer through the public API...
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/web/web_local_frame.h;l=132;drc=84e9f8cb8ba9621be941c81af04d33df743f7de4
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/web_local_frame_impl.h;l=114;drc=2b6f052e3ec785493a37c4573c972954566dbda7
[3] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/web/web_frame.h;l=104;drc=aece0d42b548b986c6e54b8ef92acaa96591dac9
[4] https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/render_frame_impl.cc;l=3346;drc=b50ce753d86531e4c8c57c2e836e50ba6340a184
[5] https://codereview.chromium.org/2837593002 is the CL that removed `WebFrameImplBase` to get an idea of what it used to look like.
web_media_player_->Shutdown();Dale CurtisSo code like this can't accidentally use `reset()`. It would mean we would need to pass the task runner to Shutdown() but maybe Shutdown() could post the `delete this` task itself then.
If we don't take your suggestion below, we don't need the complexity you suggest here. Either way is fine, it's just that the singular HTMLME destruction path needs to post. Outside of tests, HTMLME is the only owner of this class.
Can you elaborate on why you think not automatically calling Shutdown() in destruction is better? I can see the appeal with a utility interface with diverse usage, but that's not the case here.
Hmm... so it's just my feeling that if we're relying on things like "post deletion of `this` to avoid potential use-after-frees", we really ought to encapsulate that as much as possible. I agree that as-written it's safe, but it's also one of those things that seems easy to accidentally regress sometime in the future if code is refactored.
Related details
- Dale Curtis
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. |
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
Dale CurtisI don't love this; there are a lot of other GCed types that are exposed through the Blink public API. I'm not going to say it's painless, but it's not usually that unwieldy either. But I didn't investigate so I don't quite know all the roadblocks that might be involved here...
Daniel ChengA quick search doesn't show any GarbageCollected interfaces in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/ ? Can you give an example?
`WebFrame`/`WebLocalFrame` is one example. It's not marked as GCed in the public API [1], since we've decided not to expose Oilpan details outside Blink. But the actual implementation class `WebLocalFrameImpl` is garbage collected [2]; it inherits from `GarbageCollected<T>` and `WebNavigationControl` (the latter of which inherits from `WebLocalFrame`).
Blink's embedder (//content) owns the lifetime of web frames. When the lifetime of the `WebFrame` should end, the embedder calls `WebFrame::Close` [3]. Internally, this releases `WebLocalFrameImpl`'s `SelfKeepAlive` reference–from the perspective of //content, it may no longer use a `WebFrame` once it calls `Close()` on it. This is (part of) the reason `RenderFrameImpl` has `weak_this` checks [4] in various places... (the other reason is missing the `weak_this` check would simply be a use-after-free of the `RenderFrameImpl` :)
It'd be a bit trickier for `WebMediaPlayer`... unlike `WebLocalFrame`, we don't really need the `SelfKeepAlive` since the embedder / //media doesn't hang on to the `WebMediaPlayer` after creating it. The web frame bits have changed significantly over time, but way we originally "solved" it there was by having `WebFrameImplBase` [5]. For `WebMediaPlayer`... I'm actually wondering how much of `WMP` really needs to be exposed in the Blink public API. It seems like media really just needs the factory APIs to create WMP instances, and that we could probably hide all the actual subclass details in Blink... so we could even (potentially) get away with just returning an opaque `WebMediaPlayer` pointer through the public API...
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/web/web_local_frame.h;l=132;drc=84e9f8cb8ba9621be941c81af04d33df743f7de4
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/web_local_frame_impl.h;l=114;drc=2b6f052e3ec785493a37c4573c972954566dbda7
[3] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/web/web_frame.h;l=104;drc=aece0d42b548b986c6e54b8ef92acaa96591dac9
[4] https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/render_frame_impl.cc;l=3346;drc=b50ce753d86531e4c8c57c2e836e50ba6340a184
[5] https://codereview.chromium.org/2837593002 is the CL that removed `WebFrameImplBase` to get an idea of what it used to look like.
Thanks, I'll look through this. Frank indicates he took a stab at this previously and it rapidly exploded though. There are quite a few things that WMP creation needs from content/, I think it could be reflowed such that move of `MediaFactory` lives in blink, but it's a big effort.
I worry that even if we succeed in making WMP GC'd, we'll just have fragmented the problem into a number of other pieces lower in the stack. WMP has lots of non-GC'd members which call back into it via interfaces which would now suffer from the same problem we're having with WMP + GC'd objects.
web_media_player_->Shutdown();Dale CurtisSo code like this can't accidentally use `reset()`. It would mean we would need to pass the task runner to Shutdown() but maybe Shutdown() could post the `delete this` task itself then.
Daniel ChengIf we don't take your suggestion below, we don't need the complexity you suggest here. Either way is fine, it's just that the singular HTMLME destruction path needs to post. Outside of tests, HTMLME is the only owner of this class.
Can you elaborate on why you think not automatically calling Shutdown() in destruction is better? I can see the appeal with a utility interface with diverse usage, but that's not the case here.
Hmm... so it's just my feeling that if we're relying on things like "post deletion of `this` to avoid potential use-after-frees", we really ought to encapsulate that as much as possible. I agree that as-written it's safe, but it's also one of those things that seems easy to accidentally regress sometime in the future if code is refactored.
I'll see about adding a DCHECK for having shutdown called and see how much explodes with that.
Related details
- Daniel Cheng
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. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 7 comments![]( "Open in Gerrit")
Votes added by Dale Curtis
| Auto-Submit | +0 |
| Commit-Queue | +1 |
7 comments
Probably want to make this protected.
Didn't do this one in favor of just having a CHECK().
Dale CurtisSo code like this can't accidentally use `reset()`. It would mean we would need to pass the task runner to Shutdown() but maybe Shutdown() could post the `delete this` task itself then.
Daniel ChengIf we don't take your suggestion below, we don't need the complexity you suggest here. Either way is fine, it's just that the singular HTMLME destruction path needs to post. Outside of tests, HTMLME is the only owner of this class.
Can you elaborate on why you think not automatically calling Shutdown() in destruction is better? I can see the appeal with a utility interface with diverse usage, but that's not the case here.
Dale CurtisHmm... so it's just my feeling that if we're relying on things like "post deletion of `this` to avoid potential use-after-frees", we really ought to encapsulate that as much as possible. I agree that as-written it's safe, but it's also one of those things that seems easy to accidentally regress sometime in the future if code is refactored.
I'll see about adding a DCHECK for having shutdown called and see how much explodes with that.
Seems like this ended up pretty easy, but we'll see what CQ says.
Can we fix the tests?
Done
```suggestion
weak_factory_.InvalidateWeakPtrsAndDoom();
```
Otherwise people can just call `GetWeakPtr()` again.
Done
Is it hard to fix tests to do the right thing? It would be better not to have this.
Done
weak_factory_.InvalidateWeakPtrs();Dale Curtis```suggestion
weak_factory_.InvalidateWeakPtrsAndDoom();
```
Done
void Shutdown() override { weak_ptr_factory_.InvalidateWeakPtrs(); }Frank Liberato```suggestion
void Shutdown() override { weak_ptr_factory_.InvalidateWeakPtrsAndDoom(); }
```
'...AndDoom'? never heard of that one. it's a cool name, Frodo Baggins.
- Daniel Cheng
- Frank Liberato
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. |
Daniel Cheng (Gerrit)
Daniel Cheng voted Code-Review+1![]( "Open in Gerrit")
- Dale Curtis
- Frank Liberato
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. |
Dale Curtis (Gerrit)
Dale Curtis voted Auto-Submit+1![]( "Open in Gerrit")
- Frank Liberato
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. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
Dale CurtisI don't love this; there are a lot of other GCed types that are exposed through the Blink public API. I'm not going to say it's painless, but it's not usually that unwieldy either. But I didn't investigate so I don't quite know all the roadblocks that might be involved here...
Daniel ChengA quick search doesn't show any GarbageCollected interfaces in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/ ? Can you give an example?
Dale Curtis`WebFrame`/`WebLocalFrame` is one example. It's not marked as GCed in the public API [1], since we've decided not to expose Oilpan details outside Blink. But the actual implementation class `WebLocalFrameImpl` is garbage collected [2]; it inherits from `GarbageCollected<T>` and `WebNavigationControl` (the latter of which inherits from `WebLocalFrame`).
Blink's embedder (//content) owns the lifetime of web frames. When the lifetime of the `WebFrame` should end, the embedder calls `WebFrame::Close` [3]. Internally, this releases `WebLocalFrameImpl`'s `SelfKeepAlive` reference–from the perspective of //content, it may no longer use a `WebFrame` once it calls `Close()` on it. This is (part of) the reason `RenderFrameImpl` has `weak_this` checks [4] in various places... (the other reason is missing the `weak_this` check would simply be a use-after-free of the `RenderFrameImpl` :)
It'd be a bit trickier for `WebMediaPlayer`... unlike `WebLocalFrame`, we don't really need the `SelfKeepAlive` since the embedder / //media doesn't hang on to the `WebMediaPlayer` after creating it. The web frame bits have changed significantly over time, but way we originally "solved" it there was by having `WebFrameImplBase` [5]. For `WebMediaPlayer`... I'm actually wondering how much of `WMP` really needs to be exposed in the Blink public API. It seems like media really just needs the factory APIs to create WMP instances, and that we could probably hide all the actual subclass details in Blink... so we could even (potentially) get away with just returning an opaque `WebMediaPlayer` pointer through the public API...
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/web/web_local_frame.h;l=132;drc=84e9f8cb8ba9621be941c81af04d33df743f7de4
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/web_local_frame_impl.h;l=114;drc=2b6f052e3ec785493a37c4573c972954566dbda7
[3] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/web/web_frame.h;l=104;drc=aece0d42b548b986c6e54b8ef92acaa96591dac9
[4] https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/render_frame_impl.cc;l=3346;drc=b50ce753d86531e4c8c57c2e836e50ba6340a184
[5] https://codereview.chromium.org/2837593002 is the CL that removed `WebFrameImplBase` to get an idea of what it used to look like.
Thanks, I'll look through this. Frank indicates he took a stab at this previously and it rapidly exploded though. There are quite a few things that WMP creation needs from content/, I think it could be reflowed such that move of `MediaFactory` lives in blink, but it's a big effort.
I worry that even if we succeed in making WMP GC'd, we'll just have fragmented the problem into a number of other pieces lower in the stack. WMP has lots of non-GC'd members which call back into it via interfaces which would now suffer from the same problem we're having with WMP + GC'd objects.
Acknowledged
Related details
- Frank Liberato
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato voted Code-Review+1![]( "Open in Gerrit")
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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
Always post destruction of WebMediaPlayer instances
There are lots of ways that re-entrant destruction of players can
happen. Fixing them all piecemeal is fragile and making WMP garbage
collected is difficult since it's a blink/public interface. For now
just add a Shutdown mechanism and post destruction such that we
never have re-entrant destruction.
- M third_party/blink/public/platform/web_media_player.h
- M third_party/blink/public/web/modules/mediastream/web_media_player_ms.h
- M third_party/blink/renderer/core/exported/web_media_player_impl_unittest.cc
- M third_party/blink/renderer/core/html/media/html_media_element.cc
- M third_party/blink/renderer/modules/mediacapturefromelement/html_video_element_capturer_source_unittest.cc
- M third_party/blink/renderer/modules/mediastream/web_media_player_ms.cc
- M third_party/blink/renderer/modules/mediastream/web_media_player_ms_test.cc
- M third_party/blink/renderer/platform/media/web_media_player_impl.cc
- M third_party/blink/renderer/platform/media/web_media_player_impl.h
- M third_party/blink/renderer/platform/testing/empty_web_media_player.h
Code-Review: +1 by Frank Liberato, +1 by Daniel Cheng
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |