Bug 66230 – Also release media resources when media is completely loaded.

• Component: Media
• Status: RESOLVED DUPLICATE

[Hao Zheng]

DRT will not release resources of media player in the old page when opening a new page. It will only release resources when GC is needed (see stacktrace GC). But actually DRT will try to release media resources explicitly if it is not completely loaded (see stacktrace EXPLICIT). It may not be a problem on other platforms, but it will cause unstable behavior on platforms with limited resources e.g. mobile phones, especially when running consecutive tests in DRT. If we release media which is not completely loaded, it's better to also release completely loaded media.

GC:
#0  webkit_glue::WebMediaPlayerImpl::Destroy (this=0x7fffea161930) at Source/WebKit/chromium/webkit/glue/webmediaplayer_impl.cc:1004
#1  0x000000000195e01c in webkit_glue::WebMediaPlayerImpl::~WebMediaPlayerImpl (this=0x7fffea161930, __in_chrg=<value optimized out>)
   at Source/WebKit/chromium/webkit/glue/webmediaplayer_impl.cc:424
#2  0x0000000000506477 in WTF::deleteOwnedPtr<WebKit::WebMediaPlayer> (ptr=0x7fffea161930) at Source/JavaScriptCore/wtf/OwnPtrCommon.h:65
#3  0x0000000000505eb5 in WTF::OwnPtr<WebKit::WebMediaPlayer>::~OwnPtr (this=0x7fffea0a8660, __in_chrg=<value optimized out>) at Source/JavaScriptCore/wtf/OwnPtr.h:54
#4  0x0000000000503aea in WebKit::WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl (this=0x7fffea0a8640, __in_chrg=<value optimized out>)
   at Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:108
#5  0x0000000000ff31e2 in WTF::deleteOwnedPtr<WebCore::MediaPlayerPrivateInterface> (ptr=0x7fffea0a8640) at Source/JavaScriptCore/wtf/OwnPtrCommon.h:65
#6  0x0000000000ff2f63 in WTF::OwnPtr<WebCore::MediaPlayerPrivateInterface>::~OwnPtr (this=0x7fffea104af0, __in_chrg=<value optimized out>)
   at Source/JavaScriptCore/wtf/OwnPtr.h:54
#7  0x0000000000ff0f1e in WebCore::MediaPlayer::~MediaPlayer (this=0x7fffea104aa0, __in_chrg=<value optimized out>) at Source/WebCore/platform/graphics/MediaPlayer.cpp:318
#8  0x0000000000ebd891 in WTF::deleteOwnedPtr<WebCore::MediaPlayer> (ptr=0x7fffea104aa0) at Source/JavaScriptCore/wtf/OwnPtrCommon.h:65
#9  0x0000000000ebd4ef in WTF::OwnPtr<WebCore::MediaPlayer>::~OwnPtr (this=0x7ffff7e8cb40, __in_chrg=<value optimized out>) at Source/JavaScriptCore/wtf/OwnPtr.h:54
#10 0x0000000000eb41eb in WebCore::HTMLMediaElement::~HTMLMediaElement (this=0x7ffff7e8c8c0, __in_chrg=<value optimized out>) at Source/WebCore/html/HTMLMediaElement.cpp:201
#11 0x000000000211445b in WebCore::HTMLAudioElement::~HTMLAudioElement (this=0x7ffff7e8c8c0, __in_chrg=<value optimized out>) at Source/WebCore/html/HTMLAudioElement.h:38
#12 0x0000000000d943fc in WebCore::TreeShared<WebCore::ContainerNode>::removedLastRef (this=0x7ffff7e8c8c8) at Source/WebCore/platform/TreeShared.h:118
#13 0x0000000000476865 in WebCore::TreeShared<WebCore::ContainerNode>::deref (this=0x7ffff7e8c8c8) at Source/WebCore/platform/TreeShared.h:79
#14 0x0000000001569a72 in WebCore::DOMDataStore::weakNodeCallback (value=..., domObject=0x7ffff7e8c8c0) at Source/WebCore/bindings/v8/DOMDataStore.cpp:153
#15 0x0000000000a47232 in v8::internal::GlobalHandles::Node::PostGarbageCollectionProcessing (this=0x7ffff7e5cc00, isolate=0x7ffff7e44000, global_handles=0x7ffff7e74000)
   at Source/WebKit/chromium/v8/src/global-handles.cc:233
#16 0x0000000000a45b9d in v8::internal::GlobalHandles::PostGarbageCollectionProcessing (this=0x7ffff7e74000, collector=v8::internal::MARK_COMPACTOR)
   at Source/WebKit/chromium/v8/src/global-handles.cc:555
#17 0x0000000000a57ad0 in v8::internal::Heap::PerformGarbageCollection (this=0x7ffff7e44098, collector=v8::internal::MARK_COMPACTOR, tracer=0x7fffffffaa40)
   at Source/WebKit/chromium/v8/src/heap.cc:771
#18 0x0000000000a57089 in v8::internal::Heap::CollectGarbage (this=0x7ffff7e44098, space=v8::internal::NEW_SPACE, collector=v8::internal::MARK_COMPACTOR)
   at Source/WebKit/chromium/v8/src/heap.cc:508
#19 0x0000000000a11789 in v8::internal::Heap::CollectGarbage (this=0x7ffff7e44098, space=v8::internal::NEW_SPACE) at Source/WebKit/chromium/v8/src/heap-inl.h:427
#20 0x0000000000a4acea in v8::internal::SetProperty (object=..., key=..., value=..., attributes=READ_ONLY, strict_mode=v8::internal::kNonStrictMode)
   at Source/WebKit/chromium/v8/src/handles.cc:269
#21 0x0000000000b883cc in v8::internal::Runtime::SetObjectProperty (isolate=0x7ffff7e44000, object=..., key=..., value=..., attr=READ_ONLY,     strict_mode=v8::internal::kNonStrictMode) at Source/WebKit/chromium/v8/src/runtime.cc:4135
#22 0x0000000000b88b62 in v8::internal::Runtime_SetProperty (args=..., isolate=0x7ffff7e44000) at Source/WebKit/chromium/v8/src/runtime.cc:4270

EXPLICIT:
#10 0x002fb5a0 in ~MediaPlayer (this=0x12adb18, __in_chrg=<value optimized out>) at third_party/WebKit/Source/WebCore/platform/graphics/MediaPlayer.cpp:318
#11 0x0000cb2a in WTF::deleteOwnedPtr<CppBoundClass::Callback> (ptr=0x12adb18) at third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtrCommon.h:65
#12 0x002a6f20 in WTF::OwnPtr<WebCore::MediaPlayer>::clear (this=0x123b840) at third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtr.h:99
#13 WebCore::HTMLMediaElement::userCancelledLoad (this=0x123b840) at third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp:2343
#14 0x002a9212 in WebCore::HTMLMediaElement::stop (this=0x123b840) at third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp:2388
#15 0x0028e07c in WebCore::ScriptExecutionContext::stopActiveDOMObjects (this=0x1247c58) at third_party/WebKit/Source/WebCore/dom/ScriptExecutionContext.cpp:271
#16 0x00276930 in WebCore::Document::detach (this=0x1247bd0) at third_party/WebKit/Source/WebCore/dom/Document.cpp:1787
#17 0x003edc1e in WebCore::Frame::setView (this=0x12334c8, view=...) at third_party/WebKit/Source/WebCore/page/Frame.cpp:275
#18 0x00024104 in WebKit::WebFrameImpl::createFrameView (this=0x1233378) at third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:2308
#19 0x0003a71c in WebKit::FrameLoaderClientImpl::makeDocumentView (this=<value optimized out>)
   at third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:254
#20 WebKit::FrameLoaderClientImpl::transitionToCommittedForNewPage (this=<value optimized out>)
   at third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1395
#21 0x003c8ecc in WebCore::FrameLoader::transitionToCommitted (this=0x1233508, cachedPage=...) at third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:1892
#22 0x003c8fca in WebCore::FrameLoader::commitProvisionalLoad (this=0x1233508) at third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:1739
#23 0x003c056a in WebCore::DocumentLoader::commitIfReady (this=<value optimized out>) at third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:279
#24 0x003c058c in WebCore::DocumentLoader::commitLoad (this=0x12c22d8,
   data=0x12c9b20 "<body>\n<p>Test that Audio(\"url\") constructor loads the specified resource.</p>\n<script src=media-file.js></script>\n<script src=video-test.js></script>\n<script>\n    var audioFile = findMediaFile(\"audio"..., length=506) at third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:300
#25 0x003c060e in WebCore::DocumentLoader::receivedData (this=0x12c22d8,
   data=0x12c9b20 "<body>\n<p>Test that Audio(\"url\") constructor loads the specified resource.</p>\n<script src=media-file.js></script>\n<script src=video-test.js></script>\n<script>\n    var audioFile = findMediaFile(\"audio"..., length=506) at third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:334
#26 0x003cdc6e in WebCore::MainResourceLoader::addData (this=0x12c3100,
   data=0x12c9b20 "<body>\n<p>Test that Audio(\"url\") constructor loads the specified resource.</p>\n<script src=media-file.js></script>\n<script src=video-test.js></script>\n<script>\n    var audioFile = findMediaFile(\"audio"..., length=506, allAtOnce=<value optimized out>)
   at third_party/WebKit/Source/WebCore/loader/MainResourceLoader.cpp:168
#27 0x003d2270 in WebCore::ResourceLoader::didReceiveData (this=0x12c3100,
   data=0x12c9b20 "<body>\n<p>Test that Audio(\"url\") constructor loads the specified resource.</p>\n<script src=media-file.js></script>\n<script src=video-test.js></script>\n<script>\n    var audioFile = findMediaFile(\"audio"..., length=506, encodedDataLength=<value optimized out>, allAtOnce=false)
   at third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:304
#28 0x003cdc4a in WebCore::MainResourceLoader::didReceiveData (this=0x12c3100,
   data=0x12c9b20 "<body>\n<p>Test that Audio(\"url\") constructor loads the specified resource.</p>\n<script src=media-file.js></script>\n<script src=video-test.js></script>\n<script>\n    var audioFile = findMediaFile(\"audio"..., length=506, encodedDataLength=-1, allAtOnce=false)
   at third_party/WebKit/Source/WebCore/loader/MainResourceLoader.cpp:464
#29 0x003d2098 in WebCore::ResourceLoader::didReceiveData (this=0x12c3100,
   data=0x12c9b20 "<body>\n<p>Test that Audio(\"url\") constructor loads the specified resource.</p>\n<script src=media-file.js></script>\n<script src=video-test.js></script>\n<script>\n    var audioFile = findMediaFile(\"audio"..., length=506, encodedDataLength=-1) at third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:462
#30 0x0057306e in WebCore::ResourceHandleInternal::didReceiveData (this=0x12c3870,
   data=0x12c9b20 "<body>\n<p>Test that Audio(\"url\") constructor loads the specified resource.</p>\n<script src=media-file.js></script>\n<script src=video-test.js></script>\n<script>\n    var audioFile = findMediaFile(\"audio"..., dataLength=506, encodedDataLength=-1) at third_party/WebKit/Source/WebKit/chromium/src/ResourceHandle.cpp:172
#31 0x005372a0 in webkit_glue::WebURLLoaderImpl::Context::OnReceivedData (this=0x128a148,
   data=0x12c9b20 "<body>\n<p>Test that Audio(\"url\") constructor loads the specified resource.</p>\n<script src=media-file.js></script>\n<script src=video-test.js></script>\n<script>\n    var audioFile = findMediaFile(\"audio"..., data_length=506, encoded_data_length=<value optimized out>) at webkit/glue/weburlloader_impl.cc:619
#32 0x0056c376 in NotifyReceivedData (this=<value optimized out>, bytes_read=506) at webkit/tools/test_shell/simple_resource_loader_bridge.cc:280
#33 0x0056a62c in DispatchToMethod<<unnamed>::RequestProxy, void (<unnamed>::RequestProxy::*)(int), int> (this=0x1fa) at ./base/tuple.h:551
#34 Run (this=0x1fa) at ./base/task.h:338
#35 0x0012651c in Run (this=0x12c8c40) at base/message_loop.cc:109
#36 0x001265c6 in DoInvoke (base=<value optimized out>) at ./base/bind_internal.h:595

[Hao Zheng]

Created attachment 103918 [details]
Proposed patch.

[Alexey Proskuryakov]

Does this mean that the resource will be unavailable in browser when going back to a page that contains it?

[Eric Carlson]

(In reply to comment #2)
> Does this mean that the resource will be unavailable in browser when going back to a page that contains it?

Yes it does. With this patch leaving a page will cause all HTMLMediaElement state to be reset and the media engine to be deleted, so when a user goes back the media engine must be recreated, media data will reloaded, all events will be retired, etc.

The existing logic is from http://www.w3.org/TR/html5/video.html#dfnReturnLink-2, see the paragraph after "If the media data fetching process is aborted by the user".

[Eric Carlson]

Comment on attachment 103918 [details]
Proposed patch.

Changing the behavior everywhere, in a way that breaks compatibility with the spec, is the wrong way to fix a problem that only happens in DRT on some platforms.

[Hin-Chung Lam]

(In reply to comment #4)
> (From update of attachment 103918 [details])
> Changing the behavior everywhere, in a way that breaks compatibility with the spec, is the wrong way to fix a problem that only happens in DRT on some platforms.

I suppose that a media resource never is loaded unless it's from a local file system. This effectively means that in all cases except for file:// URLs MediaPlayer is deleted.

If this is the final result then why do we treat file:// differently? This seems to fall into a grey area where the spec doesn't specify the behavior regarding media resources / cache when user leave the page.

I think we should just treat the logic for file:// the same as other URLs. Logic in this piece of code seems to be coming the old spec where there was a loaded event.

[Eric Carlson]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 103918 [details] [details])
> > Changing the behavior everywhere, in a way that breaks compatibility with the spec, is the wrong way to fix a problem that only happens in DRT on some platforms.
> 
> I suppose that a media resource never is loaded unless it's from a local file system. This effectively means that in all cases except for file:// URLs MediaPlayer is deleted.
> 

  No, what makes you say that? 

  HTMLMediaElement::m_completelyLoaded is set when the media engine reports that the networkState is MediaPlayer::Loaded. This can potentially happen for any file, it is up to the media engine.

> If this is the final result then why do we treat file:// differently? This seems to fall into a grey area where the spec doesn't specify the behavior regarding media resources / cache when user leave the page.
> 
  I don't think the spec is at all vague. It says to run the steps we have in HTMLMediaElement::userCancelledLoad if the "fetching process is aborted by the user". To me this means the steps should not be run if the fetching process is not aborted, hence if the data has all been fetched we do not run it.

> I think we should just treat the logic for file:// the same as other URLs. Logic in this piece of code seems to be coming the old spec where there was a loaded event.

  As noted this is not caused by file: urls, and it does not come from the old spec. 

  I think this is a usability issue. When navigating back to a page it is a good thing if the page can reload without having to refetch the media, eg. the user's place in the media is not reset so they can resume watching/listening where they were, etc.

  If you don't like this behavior, change your media engine so the networkState never reaches MediaPlayer::Loaded.

[Hin-Chung Lam]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 103918 [details] [details] [details])
> > > Changing the behavior everywhere, in a way that breaks compatibility with the spec, is the wrong way to fix a problem that only happens in DRT on some platforms.
> > 
> > I suppose that a media resource never is loaded unless it's from a local file system. This effectively means that in all cases except for file:// URLs MediaPlayer is deleted.
> > 
> 
>   No, what makes you say that? 
> 
>   HTMLMediaElement::m_completelyLoaded is set when the media engine reports that the networkState is MediaPlayer::Loaded. This can potentially happen for any file, it is up to the media engine.
> 
> > If this is the final result then why do we treat file:// differently? This seems to fall into a grey area where the spec doesn't specify the behavior regarding media resources / cache when user leave the page.
> > 
>   I don't think the spec is at all vague. It says to run the steps we have in HTMLMediaElement::userCancelledLoad if the "fetching process is aborted by the user". To me this means the steps should not be run if the fetching process is not aborted, hence if the data has all been fetched we do not run it.
> 
> > I think we should just treat the logic for file:// the same as other URLs. Logic in this piece of code seems to be coming the old spec where there was a loaded event.
> 
>   As noted this is not caused by file: urls, and it does not come from the old spec. 
> 
>   I think this is a usability issue. When navigating back to a page it is a good thing if the page can reload without having to refetch the media, eg. the user's place in the media is not reset so they can resume watching/listening where they were, etc.
> 
>   If you don't like this behavior, change your media engine so the networkState never reaches MediaPlayer::Loaded.

If the media resource is loaded from a data: URL, going to a loaded state is the right thing to do since there's no network activity and no progress event. When a site loads some data: URL objects for games, and user leaves the page that would leave a lot of media engines remain in memory, which can be quite heavy.

Currently there's no way for a media engine to know that it can clean up safely once it reached loaded state, what about we call cancelLoad() and let the media engine to decide what to do with its internal resources?

[Eric Carlson]

(In reply to comment #7)
> If the media resource is loaded from a data: URL, going to a loaded state is the right thing to do since there's no network activity and no progress event. When a site loads some data: URL objects for games, and user leaves the page that would leave a lot of media engines remain in memory, which can be quite heavy.
> 
> Currently there's no way for a media engine to know that it can clean up safely once it reached loaded state, what about we call cancelLoad() and let the media engine to decide what to do with its internal resources?

That isn't a good idea, cancelLoad() is currently called from HTMLMediaElement::prepareForLoad to tell a media engine to clean up in preparation for a new url. Apple's media engines dispose of all assets in cancelLoad, so all state is lost. I assume at least some others do the same.

We could add a new media engine function that is called when its document becomes active/inactive.

[Hao Zheng]

Created attachment 104161 [details]
Proposed patch 2.

Good idea. Actually, I noticed Document::detach() said:

    // Send out documentWillBecomeInactive() notifications to registered elements,
    // in order to stop media elements
    documentWillBecomeInactive();

But HTMLMediaElement has not overridden related methods. Please help review my new patch.

[Eric Carlson]

Comment on attachment 104161 [details]
Proposed patch 2.

View in context: https://bugs.webkit.org/attachment.cgi?id=104161&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:2654
> +void HTMLMediaElement::documentWillBecomeInactive()
> +{
> +#if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
> +    LOG(Media, "HTMLMediaElement::documentWillBecomeInactive");
> +    if (m_player)
> +        m_player->willBecomeInactive();
> +#endif
> +}
> +
> +void HTMLMediaElement::documentDidBecomeActive()
> +{
> +#if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
> +    LOG(Media, "HTMLMediaElement::documentDidBecomeActive");
> +    if (m_player)
> +        m_player->didBecomeActive();
> +#endif
> +}
> +

These new methods are unnecessary. documentWillBecomeInactive is called by Document::detach, *after* it calls stopActiveDOMObjects(). HTMLMediaElement is an ActiveDOMObject.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:243
> +void WebMediaPlayerClientImpl::willBecomeInactive()
> +{
> +    if (m_webMediaPlayer.get())
> +        m_webMediaPlayer->willBecomeInactive();
> +}
> +
> +void WebMediaPlayerClientImpl::didBecomeActive()
> +{
> +    if (m_webMediaPlayer.get())
> +        m_webMediaPlayer->didBecomeActive();
> +}
> +

I assume that the Chromium media player sets network and ready states correctly when it unloads cached media data? Where are the tests to show that the behavior is correct.?

[Hin-Chung Lam]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 103918 [details] [details] [details])
> > > Changing the behavior everywhere, in a way that breaks compatibility with the spec, is the wrong way to fix a problem that only happens in DRT on some platforms.
> > 
> > I suppose that a media resource never is loaded unless it's from a local file system. This effectively means that in all cases except for file:// URLs MediaPlayer is deleted.
> > 
> 
>   No, what makes you say that? 
> 
>   HTMLMediaElement::m_completelyLoaded is set when the media engine reports that the networkState is MediaPlayer::Loaded. This can potentially happen for any file, it is up to the media engine.
> 
> > If this is the final result then why do we treat file:// differently? This seems to fall into a grey area where the spec doesn't specify the behavior regarding media resources / cache when user leave the page.
> > 
>   I don't think the spec is at all vague. It says to run the steps we have in HTMLMediaElement::userCancelledLoad if the "fetching process is aborted by the user". To me this means the steps should not be run if the fetching process is not aborted, hence if the data has all been fetched we do not run it.
> 
> > I think we should just treat the logic for file:// the same as other URLs. Logic in this piece of code seems to be coming the old spec where there was a loaded event.
> 
>   As noted this is not caused by file: urls, and it does not come from the old spec. 
> 
>   I think this is a usability issue. When navigating back to a page it is a good thing if the page can reload without having to refetch the media, eg. the user's place in the media is not reset so they can resume watching/listening where they were, etc.
> 
>   If you don't like this behavior, change your media engine so the networkState never reaches MediaPlayer::Loaded.

(In reply to comment #8)
> (In reply to comment #7)
> > If the media resource is loaded from a data: URL, going to a loaded state is the right thing to do since there's no network activity and no progress event. When a site loads some data: URL objects for games, and user leaves the page that would leave a lot of media engines remain in memory, which can be quite heavy.
> > 
> > Currently there's no way for a media engine to know that it can clean up safely once it reached loaded state, what about we call cancelLoad() and let the media engine to decide what to do with its internal resources?
> 
> That isn't a good idea, cancelLoad() is currently called from HTMLMediaElement::prepareForLoad to tell a media engine to clean up in preparation for a new url. Apple's media engines dispose of all assets in cancelLoad, so all state is lost. I assume at least some others do the same.
> 
> We could add a new media engine function that is called when its document becomes active/inactive.

That seems to be a better solution in terms of flexibility for the media engine to unload resources if it chooses to.

Also my other question is that instead of a m_player.clear(), should it be a cancelLoad()? Conceptually doing a destruction seems to be doing more than just cancel the resource fetch.

[Eric Carlson]

(In reply to comment #11)
> 
> Also my other question is that instead of a m_player.clear(), should it be a cancelLoad()? Conceptually doing a destruction seems to be doing more than just cancel the resource fetch.

The problem that while they are conceptually very similar, clear() deletes the player so *everything* gets cleaned up and all state is reset. cancelLoad do the same, but testing that will be difficult because AFIK not all ports have leaks bots.

[Adam Barth]

*** This bug has been marked as a duplicate of bug 89720 ***