After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762906 - epiphany does not respect restore tabs preference
epiphany does not respect restore tabs preference
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
3.19.x
Other Linux
: Normal blocker
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
: 763482 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2016-03-01 08:01 UTC by Hussam Al-Tayeb
Modified: 2016-03-22 01:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Check for CRASHED policy when resuming session (1.50 KB, patch)
2016-03-01 13:19 UTC, Gabriel Ivașcu
needs-work Details | Review
session: Check restore-session-policy in ephy_session_save() (2.10 KB, patch)
2016-03-05 20:35 UTC, Michael Catanzaro
accepted-commit_after_freeze Details | Review
session: Fix EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED (1.59 KB, patch)
2016-03-05 20:35 UTC, Michael Catanzaro
none Details | Review
session: Fix EPHY_PREFS_RESTORE_SESSION_POLICY_CRASHED (1.71 KB, patch)
2016-03-05 20:42 UTC, Michael Catanzaro
committed Details | Review

Description Hussam Al-Tayeb 2016-03-01 08:01:43 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.
Comment 1 Gabriel Ivașcu 2016-03-01 13:17:57 UTC
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.
Comment 2 Gabriel Ivașcu 2016-03-01 13:19:51 UTC
Created attachment 322755 [details] [review]
Check for CRASHED policy when resuming session

Here's my proposed fix for this.
Comment 3 Hussam Al-Tayeb 2016-03-01 14:31:24 UTC
(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!
Comment 4 Michael Catanzaro 2016-03-01 14:39:41 UTC
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?
Comment 5 Michael Catanzaro 2016-03-01 14:40:43 UTC
(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....)
Comment 6 Gabriel Ivașcu 2016-03-01 16:18:06 UTC
(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?
Comment 7 Michael Catanzaro 2016-03-01 17:09:35 UTC
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?
Comment 8 Hussam Al-Tayeb 2016-03-01 17:20:18 UTC
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.
Comment 9 Gabriel Ivașcu 2016-03-04 21:01:07 UTC
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.
Comment 10 Michael Catanzaro 2016-03-05 20:35:11 UTC
By the way, Hussam, I notice you've been reporting a lot of GNOME bugs recently. Thanks, it's a big help.
Comment 11 Michael Catanzaro 2016-03-05 20:35:32 UTC
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.
Comment 12 Michael Catanzaro 2016-03-05 20:35:36 UTC
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.
Comment 13 Michael Catanzaro 2016-03-05 20:42:14 UTC
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.
Comment 14 Hussam Al-Tayeb 2016-03-05 20:55:28 UTC
(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.
Comment 15 Hussam Al-Tayeb 2016-03-08 16:08:59 UTC
(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 16 Michael Catanzaro 2016-03-09 15:37:47 UTC
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
Comment 17 Michael Catanzaro 2016-03-11 01:32:39 UTC
*** Bug 763482 has been marked as a duplicate of this bug. ***
Comment 18 Michael Catanzaro 2016-03-20 14:36:34 UTC
(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?
Comment 19 Carlos Garcia Campos 2016-03-21 07:05:14 UTC
(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.
Comment 20 Michael Catanzaro 2016-03-22 01:00:02 UTC
Attachment 323169 [details] pushed as 006ae7d - session: Check restore-session-policy in ephy_session_save()