Implemented MediaElement ObjectDisposedException fix #1870
Conversation
|
|
This comment was marked as outdated.
This comment was marked as outdated.
| @@ -52,10 +52,14 @@ protected void ExtractMetadata(MediaMetadataRetriever retriever) | |||
|
|
|||
| public override async void SetVideoURI(global::Android.Net.Uri? uri, IDictionary<string, string>? headers) | |||
| { | |||
| base.SetVideoURI(uri, headers); | |||
There was a problem hiding this comment.
if we cache the url in a local variable, does it work?
var localUri = uri;
// rest of the code use `localUri`There was a problem hiding this comment.
It should work without any issues, but can you elaborate why we should do that? The call to the base method throws an exception (if the object is disposed), so it wouldn't matter if we cache the argument.
There was a problem hiding this comment.
That will avoid the usage of a disposed member. I looked into the SetMetadata method and can't see any dispose logical there... So it's something else that's disposing the URL, probably because we're using an async void and it's not waited.
There was a problem hiding this comment.
Take a look here. The uri that is passed down to SetVideoURI() is never disposed. To elaborate, MediaElement is working correctly unless you close the parent page while it's loading metadata, which will Dispose() the renderer and in turn dispose FormsVideoView. In the old implementation, metadata awaiting was before the base method call. If the user closes the page while we are awaiting metadata updates, then the next call to base.SetVideoURI will throw and since we are in a async void method we can't catch any exceptions outside. Of course, we can do this after awaiting:
| base.SetVideoURI(uri, headers); | |
| try | |
| { | |
| base.SetVideoURI(uri, headers); | |
| } | |
| catch(ObjectDisposedException) { } |
but that is bad code in my opinion.
I caught this bug by accident, by opening and closing the page really fast. After rearranging them, there were no more exceptions. The other bug (not set to null after disposing) was more frequent since the MetadataRetrieved event was raised before the base call and the renderer tried to access the disposed instance first.
There was a problem hiding this comment.
@anvuks I believe that your PR fixes the issue. I just want to make sure that your PR doesn't cause any side effects. I'll test it tomorrow and merge it if everything works during my test. Also, I invite @peterfoot to take a look on this if he can
There was a problem hiding this comment.
@pictos @anvuks can you guys tell me where can i find the file that need to be edited by adding try and catch ???
|
@anvuks before we merge this you need to assign the CLA, can you do that please? |
|
@pictos I already did when I submitted this PR, but for some reason the bot says it's not signed. Does it usually take this long or do I have to do something else? I've also clicked on recheck link but nothing happens. |
|
@anvuks sometimes it gets crazy hehe |
|
I have closed and reopened this PR to try to fix pending cla issue by triggering the PR webhook without any luck and just noticed that microsoft-cla bot's comment has been overwritten by the net-foundation-cla bot with the same If it's okay with you, can you merge this PR? |
|
As far as the CLA goes: one is green so that should be all good! :) |
|
So when will this be approved to be merged? |
when will this be merged? |
|
@brettnguyen It should be merged as soon as a maintainer approves my changes. If this fix is critical to your app, you are going to have to use a nightly release until the next stable release. @pictos CLA issues have been resolved, so if you have the time can you merge this please? 😁 |
|
@anvuks I'll take a look this week asap |
|
@anvuks @pictos is there a workaround I can use in the meantime while I wait for this all to get checked out? |
|
@pictos Thanks! Don't worry about it, I know the CommunityToolkit maintainers are doing it all in their free time and life sometimes gets hectic. I'll be contributing more for sure 😀 |
Description of Bug
FormsVideoView instance was never set to null after disposing, which would sometimes lead to accessing the disposed object and throwing ObjectDisposedException.
I have also rearranged method calls in SetVideoURI to avoid calling the base method after the await, during which the instance could get disposed because the overridden method is async void. The only downside of this approach I could think of is that the metadata (video size and duration) may update after the video has already started. Maybe we could revert this and override Dispose and set a private field
isDisposed = trueand then after the await check ifisDisposed == falseto call the base method?Issues Fixed
Behavioral Changes
PR Checklist
approved