GNOME Bugzilla – Bug 762906
epiphany does not respect restore tabs preference
Last modified: 2016-03-22 01:00:02 UTC
Even when I disable remember previous tabs on startup preference in options, epiphany still loads them on next startup. I launched epiphany from a terminal to check for error messages. There was nothing weird there. It is simply not respecting the preference.
I believe this is happening because when you toggle the option in Preferences dialog the policy is switched between EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED and EPHY_PREFS_RESTORE_SESSION_POLICY_ALWAYS, but the policy checked in ephy_session_resume() is EPHY_PREFS_RESTORE_SESSION_POLICY_NEVER. So the conditional will never match the actual policy and the session will always be restored regarding the option in Preferences.
Created attachment 322755 [details] [review] Check for CRASHED policy when resuming session Here's my proposed fix for this.
(In reply to Gabriel Ivascu from comment #2) > Created attachment 322755 [details] [review] [review] > Check for CRASHED policy when resuming session > > Here's my proposed fix for this. This fixed the issue for me. Thank you!
Review of attachment 322755 [details] [review]: I think we should continue to clobber this file if the setting is never. Also, doesn't this break restoring the session if a crash did occur?
(Changing priority since this is a new setting, we can't add a new setting that is completely broken. Looks like I didn't test this properly....)
(In reply to Michael Catanzaro from comment #4) > Review of attachment 322755 [details] [review] [review]: > > I think we should continue to clobber this file if the setting is never. > > Also, doesn't this break restoring the session if a crash did occur? What do you mean?
If there is a UI process crash, then when reopening the browser, you delete the session rather than reading it, so it can't know if it crashed and can't restore the session, correct?
If there is a stored-session file, can't it be deleted on exit instead of upon reopening? Maybe the preference can simply simply toggle if epiphany deletes the session store file on exit or not. On next start, epiphany can look for the session store file. - If the session store file doesn't exist, it means the application did not crash or it is not set to save sessions upon exiting. - If the session file exists, then the application crashed (didn't have time to delete the session file) or was set to remember the previous session. The browser will then resume the saved session.
As Michael pointed on IRC, this used to work in 3.18 so the problem could only lay in one of the recent commits. Running git bisect showed me this: 9c83f5014622ecb52811c093cd02d69e06f8de49 is the first bad commit commit 9c83f5014622ecb52811c093cd02d69e06f8de49 Author: Carlos Garcia Campos <cgarcia@igalia.com> Date: Thu Jan 14 11:48:11 2016 +0100 ephy-session: always set dont_save to false in ephy_session_close() It's the only thing we are doing now, so we don't need to check the policy, it's always safe to set dont_save to FALSE when session is closed. I'll need to wait for Carlos to reply as I am not sure how to fix this.
By the way, Hussam, I notice you've been reporting a lot of GNOME bugs recently. Thanks, it's a big help.
Created attachment 323169 [details] [review] session: Check restore-session-policy in ephy_session_save() Instead of doing it in the idle, it's better to do it here before scheduling the unnecessary idle. Currently it's handled in the idle because the idle is called manually from ephy_session_close() and this saves checking it in both places, but now that we need to check it in ephy_session_close() anyway, there's no value in doing it in the idle.
Created attachment 323170 [details] [review] session: Fix EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED We accidentally broke this when adding support for WebKitWebViewSessionState. Currently EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED is treated exactly the same as EPHY_PREFS_RESTORE_SESSION_POLICY_ALWAYS. The simplest way to fix this is to consider that, if we've made it to ephy_session_close(), then we have not crashed, that's a good place to delete the session file. Otherwise, that session file had better be saved so it can be loaded if we crash, ephy_session_close() is the best place to handle this.
Created attachment 323171 [details] [review] session: Fix EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED This is an alternate version of the previous patch, which deletes the session in ephy_session_close() even if the policy is NEVER. The disadvantage of this is that it is less explicit and you won't be able to grep for EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED to see where it's handled. The advantage is that if you change the policy to NEVER in the middle of a browsing session, it will be deleted when you close the browser instead of when you next open it. I'm not sure which I prefer.
(In reply to Michael Catanzaro from comment #10) > By the way, Hussam, I notice you've been reporting a lot of GNOME bugs > recently. Thanks, it's a big help. Thank you.
(In reply to Michael Catanzaro from comment #13) > Created attachment 323171 [details] [review] [review] > session: Fix EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED > > This is an alternate version of the previous patch, which deletes the > session in ephy_session_close() even if the policy is NEVER. The > disadvantage of this is that it is less explicit and you won't be able to > grep for EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED to see where it's > handled. The advantage is that if you change the policy to NEVER in the > middle of a browsing session, it will be deleted when you close the browser > instead of when you next open it. I'm not sure which I prefer. I tried this patch. Epiphany doesn't restore the session when closed correctly and set not to. I managed to crash it by deleting an empty bookmarks folder called "all bookmarks" or something like that. It resumed the old session when I restarted epiphany.
Comment on attachment 323171 [details] [review] session: Fix EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED Thanks. Leaving the bug open for the first patch. Attachment 323171 [details] pushed as 04e7811 - session: Fix EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED
*** Bug 763482 has been marked as a duplicate of this bug. ***
(In reply to Michael Catanzaro from comment #16) > Comment on attachment 323171 [details] [review] [review] > session: Fix EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED > > Thanks. Leaving the bug open for the first patch. I guess I don't care enough about the first patch to keep this bug open; it doesn't fix anything. Still it seems like a harmless change; any reason you didn't accept it, Carlos?
(In reply to Michael Catanzaro from comment #18) > (In reply to Michael Catanzaro from comment #16) > > Comment on attachment 323171 [details] [review] [review] [review] > > session: Fix EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED > > > > Thanks. Leaving the bug open for the first patch. > > I guess I don't care enough about the first patch to keep this bug open; it > doesn't fix anything. Still it seems like a harmless change; any reason you > didn't accept it, Carlos? I think I thought you submitted two different approaches for the same fix, and only looked at the second patch because Hussam had confirmed it worked.
Attachment 323169 [details] pushed as 006ae7d - session: Check restore-session-policy in ephy_session_save()