1324883 - Media cache (HTML5) not cleared when tab closed or Ctrl+F5

Status: REOPENED

(Core :: Audio/Video: Playback, defect, P3)

Description

[pihug12]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161208153507

Steps to reproduce:

The media cache (HTML5 video) of a tab isn't cleared when this tab is closed or when I press Ctrl+F5.

OS : Windows 7 x64
Firefox : 50.1.0

Private browsing is used.


Actual results:

I see this while monitoring the C: disk usage (Windows 7 x64). After watching several videos and hitting 0 byte left, closing these tabs doesn't free space.

What release the disk usage:
- close the browser
- clear the history/cache

What doesn't release the disk usage:
- close the tab
- Ctrl+F5


Expected results:

The media cache of the empty should be freed.

Comment 1

[Benjamin Chen [:bechen]]

It's easy reproduce at local.

I have a discussion with Samael(dom team) and Junior(necko team) and trying to find out the possible solutions.
The key point is that when press ctrl+f5, we set the flags
LOAD_RELOAD_BYPASS_CACHE = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_RELOAD, nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE),
LOAD_RELOAD_BYPASS_PROXY = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_RELOAD, nsIWebNavigation::LOAD_FLAGS_BYPASS_PROXY),
So trying to pass the information to MediaElement(MediaCache) or make MediaElement to query these flags.
Now, we have 3 possible solutions:
1. Broadcast an observer event when nsDocShell::LoadURI
2. Let MediaElement listen the SHistory
3. Let MediaElement access the necko's LoadFlag

Comment 2

[Benjamin Chen [:bechen]]

Attachment: Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

Review commit: https://reviewboard.mozilla.org/r/180790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180790/

Comment 3

[Benjamin Chen [:bechen]]

Attachment: Bug 1324883 - part2: Clear the MedaiCache when receive "force-reload" observer event.

Review commit: https://reviewboard.mozilla.org/r/180792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180792/

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review185864

Hi Samael, please help to review this patch.
About the "force-reload", what's the right place to call NotifyObservers?

Comment 5

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review185864

I'm not the module owner so I'll pass this to Olli for reviewing. I'm not sure where the best place is for NotifyObservers, either... probably somewhere in InternalLoad? Let's see if Olli has some comments.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review185906

::: docshell/base/nsDocShell.cpp:11716
(Diff revision 1)
>    nsJSContext::MaybeRunNextCollectorSlice(this, JS::gcreason::DOCSHELL);
>  
> +  if (mLoadType == LOAD_RELOAD_BYPASS_PROXY_AND_CACHE) {
> +    nsCOMPtr<nsIObserverService> obsSvc = services::GetObserverService();
> +    if (obsSvc) {
> +      obsSvc->NotifyObservers(nullptr, "force-reload", (const char16_t*)&mHistoryID);

(const char16_t*)&mHistoryID

The c-cast is something to be frowned upon. Is there a better way to pass arbitrary types to observers?

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 1)
>  
>    // If we don't know the duration by this point, we assume infinity, per spec.
>    if (mMaster->mDuration.Ref().isNothing()) {
>      mMaster->mDuration = Some(TimeUnit::FromInfinity());
>    }
> -

Remove this change.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8909202 [details]
Bug 1324883 - part2: Clear the MedaiCache when receive "force-reload" observer event.

https://reviewboard.mozilla.org/r/180792/#review185908

::: dom/html/HTMLMediaElement.cpp:3781
(Diff revision 1)
>      }
> +    if (strcmp(aTopic, "force-reload") == 0) {
> +      nsID* id;
> +      id = reinterpret_cast<nsID*>(const_cast<char16_t*>(aUserData));
> +      if (id && *id == mDocShellID) {
> +        mWeak->RemoveMediaElementFromURITable();

This will not clear media cache as you expected.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review185916

Could we use nsIWebProgressListener here?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

So the patch wouldn't fix tab closing case.

Have you considered using nsIWebProgressListener? I'm not really a fan of using global observer notifications for this kind of case.

Comment 10

[Benjamin Chen [:bechen]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review185916

Thanks, I'll try it.

So I guess the code will be:
1. Let the MediaElement listen the nsIWebProgressListener.
2. In the listener, query the mLoadType in nsDocShell to see if it's a ctrl+f5.

Comment 11

[Benjamin Chen [:bechen]]

Comment on attachment 8909202 [details]
Bug 1324883 - part2: Clear the MedaiCache when receive "force-reload" observer event.

https://reviewboard.mozilla.org/r/180792/#review185908

> This will not clear media cache as you expected.

Per offline discussion, the implementation won't clear the MediaCache directly. It prevent the new mediaElement to access the old MediaCache. The path works for the bug, but I need to modify the description of it.

Comment 12

[Benjamin Chen [:bechen]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180790/diff/1-2/

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review186640

Getting closer and IMO this approach looks quite a bit better.
Iframe handling needs to be tested. It is possible that we need to add progress listener to top level docshell and iframe's own docshell.

::: dom/html/HTMLMediaElement.cpp:3818
(Diff revision 2)
> +                                            , public nsSupportsWeakReference
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  void Subscribe(HTMLMediaElement* aPtr, nsIWebProgress* webProgress) {

{ goes to its own line.

Arguments should be name aName

::: dom/html/HTMLMediaElement.cpp:3826
(Diff revision 2)
> +    if (webProgress) {
> +      webProgress->AddProgressListener(this,
> +                                       nsIWebProgress::NOTIFY_STATE_NETWORK);
> +    }
> +  }
> +  void Unsubscribe() {

{ to its own line.
Same also elsewhere

::: dom/html/HTMLMediaElement.cpp:3828
(Diff revision 2)
> +                                       nsIWebProgress::NOTIFY_STATE_NETWORK);
> +    }
> +  }
> +  void Unsubscribe() {
> +    MOZ_DIAGNOSTIC_ASSERT(mWeak);
> +    // We do not need to RemoveProgressListener because the OwnerDoc()

OwnerDoc() is never null

::: dom/html/HTMLMediaElement.cpp:3841
(Diff revision 2)
> +                              uint32_t aProgressStateFlags,
> +                              nsresult aStatus) {
> +    if ((aProgressStateFlags & STATE_IS_NETWORK) &&
> +        (aProgressStateFlags & STATE_START)) {
> +      // Query the LoadType to see if it's a ctrl+F5.
> +      nsCOMPtr<nsIDocShell> shell(do_QueryInterface(aWebProgress));

So does this all work when media element is in an iframe and the top level page is reloaded?

Do we have tests for this, testing both media elements in top level document and in iframes?

::: dom/html/HTMLMediaElement.cpp:3854
(Diff revision 2)
> +    }
> +    return NS_OK;
> +  }
> +
> +  NS_IMETHODIMP
> +  OnProgressChange(nsIWebProgress *aProgress,

* goes with the type. You have inconsistent style, in some cases it is right, and in some cases * is with the argument name

Comment 14

[Benjamin Chen [:bechen]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review186640

> So does this all work when media element is in an iframe and the top level page is reloaded?
> 
> Do we have tests for this, testing both media elements in top level document and in iframes?

I just test the iframe, unfortunately the implementation can not work.

Comment 15

[Benjamin Chen [:bechen]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180790/diff/2-3/

Comment 16

[Benjamin Chen [:bechen]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review186640

> I just test the iframe, unfortunately the implementation can not work.

Call the |GetSameTypeRootTreeItem| to get the root docshell for the iframe case.
I don't understand what is "top level document". But I guess the |GetSameTypeRootTreeItem| can also handle this case.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

In general I wonder why we need to special case reloading.
This bug is also about "close the tab" not working and I don't see the patch helping with that.
(I asked about that already in comment 9)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review187492

The more I think, the more it feels we have some other underlying issue, and this is just fixing one sub-case of that issue.
In which all cases should we call ShutdownDecoder?

::: dom/html/HTMLMediaElement.cpp:3848
(Diff revision 3)
> +      // Query the LoadType to see if it's a ctrl+F5.
> +      nsCOMPtr<nsIDocShell> shell(do_QueryInterface(aWebProgress));
> +      if (shell) {
> +        uint32_t loadType;
> +        shell->GetLoadType(&loadType);
> +        if (LOAD_RELOAD_BYPASS_PROXY_AND_CACHE == loadType && mWeak->mDecoder) {

Is it guaranteed that mWeak is non-null here?
Could you add a null check to the beginning of the method.

Comment 19

[Benjamin Chen [:bechen]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review187766

::: dom/html/HTMLMediaElement.cpp:3996
(Diff revision 3)
> +  if (docShell) {
> +    nsCOMPtr<nsIDocShellTreeItem> root;
> +    docShell->GetSameTypeRootTreeItem(getter_AddRefs(root));
> +    nsCOMPtr<nsIWebProgress> webProgress = do_GetInterface(root);
> +    mForceReloadListener->Subscribe(this, webProgress.get());

play-in-detached-document.html crash, the test case play a mediaElement without docshell

Comment 20

[Benjamin Chen [:bechen]]

(In reply to Olli Pettay [:smaug] from comment #17)
> In general I wonder why we need to special case reloading.
There is a static table |gElementTable| holding live MediaElements, so that we can share the MediaCache if there is a new MediaElement with the same uri.
When force reloading, because the old MediaElement's life cycle is overlapping with the new one, so the new MediaElement will access the old MediaCache. That's I want to hook the force reload event to the MediaElement.

> This bug is also about "close the tab" not working and I don't see the patch
> helping with that.
> (I asked about that already in comment 9)
I don't think "close the tab" is a real issue, once GC clear the MediaElement after tab closing, MediaCahe be released either.

Comment 21

[Samael Wang [:freesamael] (away for now)]

(In reply to Olli Pettay [:smaug] from comment #18)
> The more I think, the more it feels we have some other underlying issue, and
> this is just fixing one sub-case of that issue.
> In which all cases should we call ShutdownDecoder?

Had a discussion with Benjamin today. It looks that media cache is more of a sharing resource between living media elements pointing to the same URI, rather than something like necko cache to prevent re-fetching the resource on next visit.

In that case, I think ShutdownDecoder should be called on document unloading, so it roughly has the same lifecycle as the document. I'm not sure if it's safe to do it inside nsDocument::OnPageHide, or we should put something after unload event gets dispatched:
http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/layout/base/nsDocumentViewer.cpp#1430

Benjamin told me that media elements are notified by NotifyActivityChanged at multiple places:
http://searchfox.org/mozilla-central/search?q=symbol:_ZL21NotifyActivityChangedP11nsISupportsPv&redirect=false

Probably we can add a nsIDocument::NotifyUnloaded(), in which NotifyActivityChanged() gets invoked?

Let us know if you have some comments or suggestions.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Benjamin Chen [:bechen] from comment #20)
> (In reply to Olli Pettay [:smaug] from comment #17)
> > In general I wonder why we need to special case reloading.
> There is a static table |gElementTable| holding live MediaElements, so that
> we can share the MediaCache if there is a new MediaElement with the same uri.
> When force reloading, because the old MediaElement's life cycle is
> overlapping with the new one, so the new MediaElement will access the old
> MediaCache. That's I want to hook the force reload event to the MediaElement.
> 
> > This bug is also about "close the tab" not working and I don't see the patch
> > helping with that.
> > (I asked about that already in comment 9)
> I don't think "close the tab" is a real issue, once GC clear the
> MediaElement after tab closing, MediaCahe be released either.
GC/CC destroy MediaElement after reload too. And why forced reload is special to normal reload here?


Yeah, using NotifyActitivyChanged here sounds reasonable.

Need to also think how this all should work when page goes to bfcache.

Comment 23

[Benjamin Chen [:bechen]]

(In reply to Olli Pettay [:smaug] from comment #22)
> (In reply to Benjamin Chen [:bechen] from comment #20)
> > (In reply to Olli Pettay [:smaug] from comment #17)
> > > In general I wonder why we need to special case reloading.
> > There is a static table |gElementTable| holding live MediaElements, so that
> > we can share the MediaCache if there is a new MediaElement with the same uri.
> > When force reloading, because the old MediaElement's life cycle is
> > overlapping with the new one, so the new MediaElement will access the old
> > MediaCache. That's I want to hook the force reload event to the MediaElement.
> > 
> > > This bug is also about "close the tab" not working and I don't see the patch
> > > helping with that.
> > > (I asked about that already in comment 9)
> > I don't think "close the tab" is a real issue, once GC clear the
> > MediaElement after tab closing, MediaCahe be released either.
> GC/CC destroy MediaElement after reload too. And why forced reload is
> special to normal reload here?
Both reloading and force reloading, the old MediaCache won't be released because the new MediaElement holding it.
Bug 1277798 comment 0 is a example. So I hope our MediaCache can receive the force reload event.

> 
> 
> Yeah, using NotifyActitivyChanged here sounds reasonable.
> 
> Need to also think how this all should work when page goes to bfcache.

I'll try it.

Comment 24

[Benjamin Chen [:bechen]]

Attachment: Bug 1324883 - part2: Once a MediaElement receive document unload event, remove all MediaElements in gElementTable with the same uri.

Review commit: https://reviewboard.mozilla.org/r/183110/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183110/

Comment 25

[Benjamin Chen [:bechen]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180790/diff/3-4/

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review188336

::: layout/base/nsDocumentViewer.cpp:1396
(Diff revision 4)
>      nsJSContext::PokeGC(JS::gcreason::PAGE_HIDE,
>                          mDocument->GetWrapperPreserveColor(),
>                          NS_GC_DELAY * 2);
>    }
>  
>    mDocument->OnPageHide(!aIsUnload, nullptr);

So, OnPageHide already calls  EnumerateActivityObservers. And it knows whether the call is for unload (since !aIsUnload is passed as param).

Couldn't you just pass that param to EnumerateActivityObservers, similarly to what you do with NotifyUnloadEvent.


(and how should this all work with bfcache?)

::: layout/base/nsDocumentViewer.cpp:1432
(Diff revision 4)
>  
>      nsIDocument::PageUnloadingEventTimeStamp timestamp(mDocument);
>  
>      EventDispatcher::Dispatch(window, mPresContext, &event, nullptr, &status);
> +    nsIDocument* doc = GetDocument();
> +    if (doc) {

Why not just
if (mDocument) {
  mDocument->...
}

But perhaps this code isn't needed

Comment 27

[Benjamin Chen [:bechen]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180790/diff/4-5/

Comment 28

[Benjamin Chen [:bechen]]

Comment on attachment 8911709 [details]
Bug 1324883 - part2: Once a MediaElement receive document unload event, remove all MediaElements in gElementTable with the same uri.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183110/diff/1-2/

Comment 29

[Benjamin Chen [:bechen]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review188336

> So, OnPageHide already calls  EnumerateActivityObservers. And it knows whether the call is for unload (since !aIsUnload is passed as param).
> 
> Couldn't you just pass that param to EnumerateActivityObservers, similarly to what you do with NotifyUnloadEvent.
> 
> 
> (and how should this all work with bfcache?)

What's the problem about bfcache?

If there are two MediaElements with the same URI, but one MediaElement is in bfcache, another is active and receive unload. 
The MediaElement in bfcache still access to the old MediaCache, but the other one won't access old MediaCache.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

What happens when a page is evicted from bfcache?

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

I mean, there is just one page using some media uri, then that page goes to bfcache.
I assume the media is still cached. Then, later the bfcache is evicted. Is the cache cleared only once GC/CC run? If so, why the same can't happen with reloads and such?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

https://reviewboard.mozilla.org/r/180790/#review188676

Comment 33

[Benjamin Chen [:bechen]]

After discuss with Samael, the nsIWebProgressListener can not handle the MediaElement with null docShell. The OnPageHide can not distinguish reload and tab closing.
So, I'm going to write a new patch that combine nsIWebProgressListener and OnPageHide. MediaElement with non-null docShell using the nsIWebProgressListener, then clear the whole MediaCache entry whose URI are the same(attachment 8911709 [details]). MediaElement with null docShell using OnPageHide then clear only its MediaCache entry(attachment 8909201 [details]).

Comment 34

[Benjamin Chen [:bechen]]

Attachment: Bug 1324883 - Shutdow the decoder when receiving nsIWebProgressListener with flag LOAD_RELOAD_BYPASS_PROXY_AND_CACHE in docshell.

We register the nsIWebProgressListener at the root docshell(GetSameTypeRootTreeItem) to handle video element embedded in iframe.

Review commit: https://reviewboard.mozilla.org/r/184494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184494/

Comment 35

[Benjamin Chen [:bechen]]

Comment on attachment 8909201 [details]
Bug 1324883 - part1: Pass the unload to MediaElement when OnPageHide(), then MediaElement can shutdown the decoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180790/diff/5-6/

Comment 36

[Benjamin Chen [:bechen]]

Comment on attachment 8911709 [details]
Bug 1324883 - part2: Once a MediaElement receive document unload event, remove all MediaElements in gElementTable with the same uri.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183110/diff/2-3/

Comment 37

[Benjamin Chen [:bechen]]

Comment on attachment 8913079 [details]
Bug 1324883 - Shutdow the decoder when receiving nsIWebProgressListener with flag LOAD_RELOAD_BYPASS_PROXY_AND_CACHE in docshell.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184494/diff/1-2/

Comment 38

[Benjamin Chen [:bechen]]

Comment on attachment 8911709 [details]
Bug 1324883 - part2: Once a MediaElement receive document unload event, remove all MediaElements in gElementTable with the same uri.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183110/diff/3-4/

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8913079 [details]
Bug 1324883 - Shutdow the decoder when receiving nsIWebProgressListener with flag LOAD_RELOAD_BYPASS_PROXY_AND_CACHE in docshell.

https://reviewboard.mozilla.org/r/184494/#review189728

I'm lost now why we need both approaches.
"MediaElement with null docShell using OnPageHide" sounds weird. Why would docshell be null, but we'd still get OnPageHide

Comment 40

[Benjamin Chen [:bechen]]

(In reply to Olli Pettay [:smaug] from comment #39)
> Comment on attachment 8913079 [details]
> Bug 1324883 - Shutdow the decoder when receiving nsIWebProgressListener with
> flag LOAD_RELOAD_BYPASS_PROXY_AND_CACHE in docshell.
> 
> https://reviewboard.mozilla.org/r/184494/#review189728
> 
> I'm lost now why we need both approaches.
> "MediaElement with null docShell using OnPageHide" sounds weird. Why would
> docshell be null, but we'd still get OnPageHide

You are right, the MediaElement with null docShell can not receive OnPageHide.
I think I should only implement nsIWebProgressListener to handle force-reload.

Comment 41

[Benjamin Chen [:bechen]]

Comment on attachment 8913079 [details]
Bug 1324883 - Shutdow the decoder when receiving nsIWebProgressListener with flag LOAD_RELOAD_BYPASS_PROXY_AND_CACHE in docshell.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184494/diff/2-3/

Comment 42

[Benjamin Chen [:bechen]]

Comment on attachment 8911709 [details]
Bug 1324883 - part2: Once a MediaElement receive document unload event, remove all MediaElements in gElementTable with the same uri.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183110/diff/4-5/

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Benjamin Chen [:bechen] from comment #33)
>  The OnPageHide can not distinguish reload
> and tab closing.
Does it need to?

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8913079 [details]
Bug 1324883 - Shutdow the decoder when receiving nsIWebProgressListener with flag LOAD_RELOAD_BYPASS_PROXY_AND_CACHE in docshell.

https://reviewboard.mozilla.org/r/184494/#review190120

I guess this is ok, but it isn't clear to me why the OnPageHide approach didn't work. Could you explain.

::: dom/html/HTMLMediaElement.cpp:4001
(Diff revision 3)
> +  if (docShell) {
> +    mForceReloadListener = new ForceReloadListener();
> +    nsCOMPtr<nsIDocShellTreeItem> root;
> +    docShell->GetSameTypeRootTreeItem(getter_AddRefs(root));
> +    nsCOMPtr<nsIWebProgress> webProgress = do_GetInterface(root);
> +    mForceReloadListener->Subscribe(this, webProgress.get());

No need for .get()

::: dom/html/HTMLMediaElement.cpp:4018
(Diff revision 3)
> +    nsCOMPtr<nsIDocShellTreeItem> root;
> +    docShell->GetSameTypeRootTreeItem(getter_AddRefs(root));
> +    webProgress = do_GetInterface(root);
> +  }
> +  if (mForceReloadListener) {
> +    mForceReloadListener->Unsubscribe(webProgress.get());

No need for .get()

Comment 45

[Benjamin Chen [:bechen]]

(In reply to Olli Pettay [:smaug] from comment #44)
> Comment on attachment 8913079 [details]
> Bug 1324883 - Shutdow the decoder when receiving nsIWebProgressListener with
> flag LOAD_RELOAD_BYPASS_PROXY_AND_CACHE in docshell.
> 
> https://reviewboard.mozilla.org/r/184494/#review190120
> 
> I guess this is ok, but it isn't clear to me why the OnPageHide approach
> didn't work. Could you explain.
> 

If there are two MediaElements(A, B) point to the same uri(U), one process, two tabs.
When the first MediaElement receive force reload or tab closing:
1. If we only clear the A-U mapping, the cache still alive because B-U mapping, then the new MediaElement still access old cache. => Not working if the developer change the content file but remain the same uri.
2.If we clear A-U and B-U mappings, then the new MediaElement will download the cache again.
=> The new MediaElement download the content again.
That's why we need to distinguish the force-reload and tab closing for OnPageHide approach.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Sorry, explain again. What is the behavior we get with OnPageHide and what is the behavior we get with nsIWebProgressListener (both in tab close and reload cases) and what is the problem with those?

Comment 47

[Benjamin Chen [:bechen]]

(In reply to Olli Pettay [:smaug] from comment #46)
> Sorry, explain again. What is the behavior we get with OnPageHide and what
> is the behavior we get with nsIWebProgressListener (both in tab close and
> reload cases) and what is the problem with those?

Every time we new a MediaElement, the MediaElement will query the mapping table trying to clone/share the MediaCache.
When a MediaElement start loading/playing, it will add its mapping into the table. The original implementation remove its own mapping when shutdownDecoder or destructor.

If there are two MediaElements A and B point to the same uri(U), one process, two tabs.
OnPageHide approach: When receive OnPageHide, the implementation can only be "remove mapping" or "remove all mappings", so the cases are:
-Reload A case:
-- implementation remove A-U mapping: The new MediaElement A'(generated by A reload) hit B-U mapping, so A' access the same MediaCache as B => Not working if the developer change the content file but remain the same uri.
-- implementation remove A-U B-U mappings: The new MediaElement A' will download the content again => works for the developer change the content file.

-tab close A case:
-- implementation remove A-U mapping: works good. MediaElement A closing so remove A-U mapping only. Keeps the B-U mapping alive.
-- implementation remove A-U B-U mappings: no good. Since the B-U mapping be cleared, the new MediaElement C need to download the content again, performance degradation.

nsIWebProgressListener approach: 
-Reload A case:
-- implementation remove A-U B-U mappings: The new MediaElement A' will download the content again => works for the developer change the content file.

-tab close A case:
-- The Listener won't active, and the A-U mapping be cleared by A destructor. If we create a new tab MediaElement C, will hit B-U mapping.

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Benjamin Chen [:bechen] from comment #47)

> If there are two MediaElements A and B point to the same uri(U), one
> process, two tabs.
> OnPageHide approach: When receive OnPageHide, the implementation can only be
> "remove mapping" or "remove all mappings", so the cases are:
> -Reload A case:
> -- implementation remove A-U mapping: The new MediaElement A'(generated by A
> reload) hit B-U mapping, so A' access the same MediaCache as B => Not
> working if the developer change the content file but remain the same uri.
> -- implementation remove A-U B-U mappings: The new MediaElement A' will
> download the content again => works for the developer change the content
> file.
     So what if a new MediaElement is then created in B. Doesn't it get the new data
     and one MediaElement in B points to the old data and one to the new one?

Comment 49

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8913079 [details]
Bug 1324883 - Shutdow the decoder when receiving nsIWebProgressListener with flag LOAD_RELOAD_BYPASS_PROXY_AND_CACHE in docshell.

https://reviewboard.mozilla.org/r/184494/#review190532

::: dom/html/HTMLMediaElement.cpp:3822
(Diff revision 3)
> +
> +  void Subscribe(HTMLMediaElement* aPtr, nsIWebProgress* aWebProgress)
> +  {
> +    MOZ_DIAGNOSTIC_ASSERT(!mWeak);
> +    mWeak = aPtr;
> +    if (aWebProgress) {

It doesn't make sense to pass a null aWebProgress to Subscribe().

::: dom/html/HTMLMediaElement.cpp:3846
(Diff revision 3)
> +    MOZ_DIAGNOSTIC_ASSERT(mWeak);
> +    if ((aProgressStateFlags & STATE_IS_NETWORK) &&
> +        (aProgressStateFlags & STATE_START)) {
> +      // Query the LoadType to see if it's a ctrl+F5.
> +      nsCOMPtr<nsIDocShell> shell(do_QueryInterface(aWebProgress));
> +      if (shell && mWeak) {

We don't need to null-check mWeak if we assert it is not null above.

::: dom/html/HTMLMediaElement.cpp:3850
(Diff revision 3)
> +      nsCOMPtr<nsIDocShell> shell(do_QueryInterface(aWebProgress));
> +      if (shell && mWeak) {
> +        uint32_t loadType;
> +        shell->GetLoadType(&loadType);
> +        if (LOAD_RELOAD_BYPASS_PROXY_AND_CACHE == loadType && mWeak->mDecoder) {
> +          mWeak->ShutdownDecoder();

I can't see how this corresponds to:

nsIWebProgressListener approach: 
-Reload A case:
-- implementation remove A-U B-U mappings: The new MediaElement A' will download the content again => works for the developer change the content file.

mWeak->ShutdownDecoder() will only remove the A-U mapping, right?

Comment 50

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8911709 [details]
Bug 1324883 - part2: Once a MediaElement receive document unload event, remove all MediaElements in gElementTable with the same uri.

https://reviewboard.mozilla.org/r/183110/#review190536

::: dom/html/HTMLMediaElement.cpp:3713
(Diff revision 5)
>    NS_ASSERTION(MediaElementTableCount(this, mLoadingSrc) == 1,
>      "Should have a single entry for element in element table after addition");
>  }
>  
>  void
> -HTMLMediaElement::RemoveMediaElementFromURITable()
> +HTMLMediaElement::RemoveMediaElementFromURITable(bool aRemoveSameURI)

Might be worth calling it "aPurgeEntry" or "aForceClearEntry".

Comment 51

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8913079 [details]
Bug 1324883 - Shutdow the decoder when receiving nsIWebProgressListener with flag LOAD_RELOAD_BYPASS_PROXY_AND_CACHE in docshell.

https://reviewboard.mozilla.org/r/184494/#review190540

::: dom/html/HTMLMediaElement.cpp:3850
(Diff revision 3)
> +      nsCOMPtr<nsIDocShell> shell(do_QueryInterface(aWebProgress));
> +      if (shell && mWeak) {
> +        uint32_t loadType;
> +        shell->GetLoadType(&loadType);
> +        if (LOAD_RELOAD_BYPASS_PROXY_AND_CACHE == loadType && mWeak->mDecoder) {
> +          mWeak->ShutdownDecoder();

I see this is done in P2.

Comment 52

[Benjamin Chen [:bechen]]

(In reply to Olli Pettay [:smaug] from comment #48)
> (In reply to Benjamin Chen [:bechen] from comment #47)
> 
> > If there are two MediaElements A and B point to the same uri(U), one
> > process, two tabs.
> > OnPageHide approach: When receive OnPageHide, the implementation can only be
> > "remove mapping" or "remove all mappings", so the cases are:
> > -Reload A case:
> > -- implementation remove A-U mapping: The new MediaElement A'(generated by A
> > reload) hit B-U mapping, so A' access the same MediaCache as B => Not
> > working if the developer change the content file but remain the same uri.
> > -- implementation remove A-U B-U mappings: The new MediaElement A' will
> > download the content again => works for the developer change the content
> > file.
>      So what if a new MediaElement is then created in B. Doesn't it get the
> new data
>      and one MediaElement in B points to the old data and one to the new one?

Yes, the old MediaElement(B) access the old cache and the new one fetching new data.

In fact, it's another problem here, if B request new network request (the cache doesn't cache all olf content due to newtwork speed or storage size, so trigger new network request):
1. The MediaCache of B contain the old data and new data?
2. The decoder might crash?

The scenario just like: During playback, the server is changing the file content. If we can not get some information from necko, it is very hard to detect this situation at media code stack.

Comment 53

[Benjamin Chen [:bechen]]

Comment on attachment 8913079 [details]
Bug 1324883 - Shutdow the decoder when receiving nsIWebProgressListener with flag LOAD_RELOAD_BYPASS_PROXY_AND_CACHE in docshell.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184494/diff/3-4/

Comment 54

[Benjamin Chen [:bechen]]

Comment on attachment 8911709 [details]
Bug 1324883 - part2: Once a MediaElement receive document unload event, remove all MediaElements in gElementTable with the same uri.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183110/diff/5-6/

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d197f1860e6
Shutdow the decoder when receiving nsIWebProgressListener with flag LOAD_RELOAD_BYPASS_PROXY_AND_CACHE in docshell. r=jwwang,smaug
https://hg.mozilla.org/integration/autoland/rev/3e22200b7e83
part2: Once a MediaElement receive document unload event, remove all MediaElements in gElementTable with the same uri. r=jwwang

Comment 56

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9d197f1860e6
https://hg.mozilla.org/mozilla-central/rev/3e22200b7e83

Comment 57

[Chris Pearce [:cpearce (Not reading bugmail)]]

I backed this out because it caused YouTube to fail to play the next video in its playlist if a user signs into YouTube on another tab (bug 1431674).

Comment 58

[Julien Cristau [:jcristau]]

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1431674#c10 (and #c14) we should back this out of 59.

Comment 59

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e61fa00f6ebf

Comment 60

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.