1078448 - Callscreen app instances linger in bfcache after being closed

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Doug Sherk (:drs) (inactive)]

When the callscreen closes because all calls have ended, we kill the app and restart it with no calls. While investigating bug 1070066, we found that the old instances linger in the bfcache.

In particular, see the conversation beginning at bug 1070066 comment 19.

Comment 1

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

I don't know what I should say here.

Are we sure those window objects are in bfcache?

Has anyone tried to explicitly disable bfcache for callscreen?
(add dummy unload event listener)

Comment 2

[Germán Toro del Valle (:gtorodelvalle)]

I'll take the liberty of need-infoing Doug regarding comment 1 ;)

Comment 3

[:gerard-majax]

I tried adding a dumb listener on the "unload" event, to see if it helps with bug 1081714, but so far it does nothing and I still see a lot of DOMWINDOW callscreen.

Comment 4

[:gerard-majax]

Doug, I have some PR to submit, sorry for stealing this :)

Comment 5

[:gerard-majax]

Attachment: Gaia PR

With this change, I'm not able to reproduce anymore bug 1074379 and bug 1081714, as documented on bug 1074379 comment 44 and bug 1074379 comment 45.

Comment 6

[:gerard-majax]

Attachment: memory-reports b2g_debug=1 after 50 calls

We don't see anymore callscreen leaking in this memory report done after running the 50 voicemail calls UI test.

Comment 7

[:gerard-majax]

[Blocking Requested - why for this release]: if it's confirmed to be fixing bug 1081714 and bug 1074379, those two are 2.1+

Comment 8

[:gerard-majax]

(In reply to Olli Pettay [:smaug] from comment #1)
> I don't know what I should say here.
> 
> Are we sure those window objects are in bfcache?
> 
> Has anyone tried to explicitly disable bfcache for callscreen?
> (add dummy unload event listener)

Yes, that does the trick. I'm resetting this bug to track the underlying platform issue, and I'll file a followup to use this workaround for now in Gaia.

Comment 9

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

What is the underlying platform issue?

Comment 10

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

The fact the pages may go to bfcache?

Comment 13

[Johan Lorenzo [:jlorenzo]]

[Blocking Requested - why for this release]: This patch fixes bug 1074379 and bug 1081714 which were both 2.1 blockers. Carrying the status of them to that bug.

Comment 14

[Kevin Grandon :kgrandon]

Is anyone doing some investigation into the underlying platform problem? We are seeing other problems with pages not loading, like bug 1080753.

Comment 15

[Doug Sherk (:drs) (inactive)]

(In reply to Kevin Grandon :kgrandon from comment #14)
> Is anyone doing some investigation into the underlying platform problem? We
> are seeing other problems with pages not loading, like bug 1080753.

Not that I'm aware of.

Comment 16

[:gerard-majax]

No, I don't know bfcache and I'm already busy with settings.

Comment 17

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

Some answer to comment 9 and comment 10 would be nice. I really don't understand what the platform issue is here. Do we have some APIs which haven't been designed so that the page can be in bfcache, yet the implementation doesn't prevent bfcache on the pages they are used or what?
(I don't know which APIs are being used in Callscreen app)

Comment 18

[Tony Chung [:tchung]]

taking for 2.1 since it blocks a blocker.

Comment 20

[Doug Sherk (:drs) (inactive)]

We worked around this issue in bug 1083729 to fix bug 1074379, and bug 1081714. Thus I don't think this should be a blocker.

Comment 21

[Doug Sherk (:drs) (inactive)]

Based on my discussion with Olli, it seems that this is expected behavior, and I agree.

Here, we reload the Callscreen window from the System app:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/callscreen_window.js#L152

Olli says that we can remove the Callscreen iframe and re-add it back to the document. This destroys and recreates the DocShell, and the bfcache along with it.

Comment 22

[Andrew Overholt [:overholt]]

Thanks, removing the 2.1 nom.

Comment 23

[Kevin Grandon :kgrandon]

I still think we should be investigating the platform issue as a 2.1+ blocker here. This may be causing other problems in gaia.

Comment 24

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

I'm investigating this a bit, but so far I haven't really seen any real platform issue.
System app just loads a page, then about:blank, then a page ,then about:blank and so on.

But we could possibly clean up session history/bfcache in the relevant iframe had some specific
attribute. (I was first thinking not using bfcache for mozbrowser things, but apparently we're dealing with a mozbrowser iframe here, and bfcache is very useful in the normal browsing case.)
But perhaps mozapp should disabled bfcache? I never remember how mozbrowser and mozapp are related to each other (like, when do we need mozapp)

Comment 25

[Kevin Grandon :kgrandon]

Bfcache is very useful, and I think we should be moving to a state where we have *more* of it, not less. I have a hunch that bug 1080753 is being caused by the same issue here though, still investigating.

Comment 26

[Kevin Grandon :kgrandon]

Oops, forgot to mention that bug 1080753 has problems when dealing with web content, in a non-mozapp state.

Comment 27

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

So in normal context we wouldn't use bfcache for iframe, but the whole point of
mozbrowser/app is that iframe starts to behave as a top level browsing context and we do have
bfcache enabled.
Setting browser.sessionhistory.max_total_viewers to 0 should disable the whole bfcache.
Perhaps we want that in the parent process and child processes could have
browser.sessionhistory.max_total_viewers 1 (b2g.js has 1 for that).

Comment 28

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

Also, if one wants to explicitly do some bfcache eviction, there are various
nsISHistoryInternal.evict*() methods for that.

Comment 29

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

Also, for b2g we probably want shorter timeout here
http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHEntryShared.cpp#29

Comment 30

[Tamara Hills [:thills]]

Attachment: WIP patch for feedback

WIP patch for feedback.  Couple of notes:
1) removed the workaround in index.js.  
2) I have a doubt about whether the _unregisterEvents is necessary.  My concern was that we'll be leaking the events if we constantly re-register them without unregistering.

Thanks,
-tamara

Comment 31

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8513472 [details] [diff] [review]
WIP patch for feedback

Review of attachment 8513472 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/system/js/app_window.js
@@ +1011,5 @@
> +    this.constructor.REGISTERED_EVENTS.forEach(function iterator(evt) {
> +      this.debug('removing ' + evt + ' event handler ...');
> +      this.element.removeEventListener(evt, this);
> +    }, this);
> +  };

From my memory profiling, it looks like we missed this originally. I'm not sure where the additional listeners are being registered from, though. It seems that they will only be attached when we call `this.render()`. Please investigate this further.

::: apps/system/js/callscreen_window.js
@@ +165,5 @@
> +        }
> +      }
> +    }
> +
> +    this.render();

This is overly complicated. All we have to do is detach `this.browser.element` from its parent and then re-attach it.

Comment 32

[Tamara Hills [:thills]]

Attachment: WIP 1078448v3.diff

Hi Doug,

I did more investigation on eventListeners all around.  

1. After looking at the memory profiles, I don't have any evidence that we are leaking eventListeners in the Callscreen due to the changes. 

2. After looking at the memory profiles on the system, I don't believe it's necessary to unregister the eventListeners.  I removed that and did not find any evidence that we are leaking them on system and found the code to be unnecessary.  

As for the other listeners in system, each Window app that inherits itself from AppWindow.js has a list of REGISTERED_EVENTS... Callscreen's is a copy of what AttentionWindow's is.  there are close to 1000 eventListeners registered before we even make a call.  When I looked at stacktrace for AppWindow._registerEvents HomeScreenWindow was one of the ones that also called this function.

There are probably tons more eventListeners in system not coming from _registerEvents, but there is no way to get a list of all the eventListeners from the memory tool, just a count.

Thanks,

-tamara

Comment 33

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8514419 [details] [diff] [review]
WIP 1078448v3.diff

Review of attachment 8514419 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable. When we're ready to go into review with this, we should flag Etienne as well.

Comment 34

[Kevin Grandon :kgrandon]

Are we still not doing investigation into the platform issue here? Are we sure that this can't impact other apps?

Comment 35

[Tamara Hills [:thills]]

Attachment: PR

Comment 36

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8518533 [details] [review]
PR

See my comments on the PR.

(In reply to Kevin Grandon :kgrandon from comment #34)
> Are we still not doing investigation into the platform issue here? Are we
> sure that this can't impact other apps?

Setting ni on myself to file bugs and do a writeup on this.

Comment 37

[Tamara Hills [:thills]]

Attachment: Updated PR after last review

Hi Doug,

I updated the PR with changes to the test to use validate the iframe is removed/added, vs. using the spies.

To address your comment about the setTimeout.  I don't believe we need this for the functionality as I tested without it and saw no leaks and everything seemed to function fine.  It was there in the code, so I did not remove it as there could be something we're not aware of.  Also, I suspect it may have been there to help with the testing.

Thanks,

-tamara

Comment 38

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8525045 [details] [review]
Updated PR after last review

Please rebase this before we review it.

Comment 39

[Tamara Hills [:thills]]

Attachment: updated PR after rebase

Hi Doug,

This is updated PR after rebasing and retesting bug 834530 to make sure it still works after rebase.

Thanks,

-tamara

Comment 40

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8525385 [details] [review]
updated PR after rebase

Looks good! I left some nit-picky comments on the PR. Please address them and then we'll merge.

As I said, we'll have to discuss the platform side of this more. Leaving my ni for now.

Please remember to make a demo and put it on the sprint demo page:
https://wiki.mozilla.org/FirefoxOS/Comms/Dialer/Sprint/v2.1-S9#Demos

Comment 41

[Tamara Hills [:thills]]

Attachment: Update to patch -- broken test

Hi Doug,

I had a broken test in index_test.js.  This update has that fixed.

Thanks,
-tamara

Comment 42

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8526341 [details] [review]
Update to patch -- broken test

In the future, please try to remember to include changes as additional commits. I'm assuming that the only changes were to 'index_test.js'.

I left one nit comment on the PR, but it otherwise looks good.

Comment 43

[Tamara Hills [:thills]]

https://github.com/mozilla-b2g/gaia/commit/a445aec48e9260d3e4b3f6980de73b90f7f99314

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=d9f1a9b8e069

Comment 44

[Doug Sherk (:drs) (inactive)]

Backed out: https://github.com/mozilla-b2g/gaia/commit/a3e2f4d39082ffc0ee67c72c48d6e7be2486010e

For causing bug 1105747.

Comment 45

[Doug Sherk (:drs) (inactive)]

For comment 36.

Comment 46

[Doug Sherk (:drs) (inactive)]

Olli suggested calling `nsIDocument::DisableBFCaching()` from somewhere in Gecko to disable the BFCache for System iframes, which I believe would fix this issue and any others like it. Unfortunately, I don't know enough about the Gecko code that supports the System app to know where this would go. Kevin, do you know who we could talk with about this?

Comment 47

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

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #46)
> Olli suggested calling `nsIDocument::DisableBFCaching()` from somewhere in
> Gecko to disable the BFCache for System iframes,

Well, to be precise I suggested calling DisableBFCaching in the API implementation of the thing
callscreen uses. If the API implementation can't deal with pages being in bfcache, it should disable bfcache.
(I don't know which all APIs callscreen uses.).

Comment 48

[Kevin Grandon :kgrandon]

Callscreen just uses a mozapp="", no? I suppose we could potentially disable it, or maybe create some new iframe property to disable it? I don't really know that code all too well though.

I'm not sure if it's realted or not, but I've just filed bug 1106280 which I think could be due to some bfcache bug?

Comment 49

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

I mean the API which one can use to answer to a call and so.  What is actually causing the issues?
Usually web pages work just fine when they enter to bfcache.

Comment 50

[BMO Automation]

Firefox OS is not being worked on