Fix resource leaks when switching between tabs #1856
Conversation
Several fixes to the process of switching between different tabs, specially when they are showing a native UI line the New Tab. Ensure that the surface held by WindowWidget is released at the same time as the session is notified that it is no longer valid. Move setupListeners() towards the end of the setSession() because it will cause session listeners to be called synchronously with the new values and we need to have a consistent state at that point. Ensure that listeners are not set up on a session that is not the current one. Fixes #1841
| if (aSession != mSession) { | ||
| Log.w(LOGTAG, "Tried to set up listeners on a session that is not the current one."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is that even possible? If not it should be better to assert as it will unveil a more important issue
There was a problem hiding this comment.
This change seems to not have any impact on the issues either.
| @@ -1258,18 +1267,20 @@ public void setSession(@NonNull Session aSession, @OldSessionDisplayAction int a | |||
|
|
|||
| mSession = aSession; | |||
|
|
|||
| setupListeners(mSession); | |||
There was a problem hiding this comment.
This change seems to have no effect on any of the issues. Why we need it ?
| if (aSession != mSession) { | ||
| Log.w(LOGTAG, "Tried to set up listeners on a session that is not the current one."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This change seems to not have any impact on the issues either.
| @@ -634,6 +638,11 @@ public void pauseCompositor() { | |||
| return; | |||
| } | |||
|
|
|||
| // Ensure that the surface is not left in an inconsistent state. | |||
| if (mSurface != null) { | |||
There was a problem hiding this comment.
This seems the only change that fixes the issue.
|
I have done some testing without the allegedly spurious changes and, even though not strictly necessary to solve the crashes described in the issues #1841 and #1854, they are useful (at least the one about the listeners) to solve (or mitigate) an issue when switching very fast among several tabs and windows; with the leak solved, there is no crash but the web-content is shown as a black window after a few times. I could accept to merge all the changes in this PR, but perhaps we could do it in different commits. Ideally we should investigate the behavior I have described, file a new issue and make an independent PR to fix it. |
Several fixes to the process of switching between different tabs, specially when they are showing a native UI line the New Tab.
Ensure that the surface held by WindowWidget is released at the same time as the session is notified that it is no longer valid.
Move setupListeners() towards the end of the setSession() because it will cause session listeners to be called synchronously with the new values and we need to have a consistent state at that point.
Ensure that listeners are not set up on a session that is not the current one.
Fixes #1841