Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

1020: SR max_undo* & 1022: SR after crash : make inactive or remove #575

Closed
8 tasks done
Thorin-Oakenpants opened this issue Dec 6, 2018 · 16 comments
Closed
8 tasks done

Comments

@Thorin-Oakenpants
Copy link
Contributor

Thorin-Oakenpants commented Dec 6, 2018

馃敽 Suggestions

  • 1020 - remove max_windows
    • it does not control what is recorded (tabs does that), does not stop any recently closed windows (because the data is gathered with tab activity)
    • it does control how many windows to restore, but that has nothing to do with this repo's objectives. And if a user wants SR, then why change this on them
  • 1020 - fix up the remaining pref, max_tabs to properly say what it does
    • IMO not worth being active. If you want to properly control SR, then clear history on close
  • make 1022 resume from cash inactive or remove it
    • even if you wipe history on close, a crash is not a graceful exit, so the file remains, and 99% of people would want to restore from a crash, surely
  • add info to 0102
    • that SR is not used in PB Mode (see 0110) & clearing history (2803, 2804) wipes SR
  • add info to 2803/2804 (that clearing history clears SR)
    • it would be nice to have this info directly with the pref, but will only impact those who want to use SR and to do that they would have to change 0102

馃敽 ToDo:

  • Check if disabling history stops any data collection in recovery.jasonlz4 - done, it has no effect AFAICT
  • Check if wiping history does anything - done, see below
  • MOAR tests
  • even MOAR tests
@Thorin-Oakenpants
Copy link
Contributor Author

Thorin-Oakenpants commented Dec 6, 2018

OK, I did some playing around. In my main FF rather than a new profile, so it was a bit messy. And the combinations of prefs can quickly escalate. Additionally, it often took two or three open/closes to get things consistent, due to when things get triggered.


one: 2803 privacy.clearOnShutdown.history

You control the persistent local storage of SR thru the above pref. If this is set to true, then ALL data in profile\sessionstore-backups\ is wiped.

Doing it manually (2804 privacy.cpd.history) also wipes all data, but then SR files kick back in after x seconds (as per the timing pref in 1022). I did not test and do not care if it doesn't re-record already open tabs etc (I don't think it does, as the history has been wiped?).

Notes : Once you have SR working (don't clear history on close, and in my case my startup was to resume sessions), you end up with a previous restore file (it would take a couple of restarts to build that). I also got an upgrade.jsonlz4-20181114214635 file the first time round, but after clearing history in my tests, it never came back.


two: 0862: places.history.enabled

Disabling history does not stop recovery files. They are still recorded and SR as startup still works. The first time my FF just opened as normal (on a blank page), but after opening a couple of websites etc, it then seemed to behave. Like I said in the top paragraph, it might take several goes to get consistency.


three: 0804: browser.sessionhistory.max_entries

Not tested and I don't think it really matters. But max history per tab, is this respected in the recovery file, e.g. if I set it to 2, what is recorded for a tab I visited 4 sites on - just the last 2, or all 5 (yup, it includes the original page: e.g your startup/newtab page?

@claustromaniac
Copy link
Contributor

It seems @earthlng was the one who initially suggested adding browser.sessionstore.interval. I'll quote:

for the sake of SSD's we should probably enable this with a reasonably high value. (I have 60000, but even 30sec is much better than the default 15sec)
https://www.servethehome.com/firefox-is-eating-your-ssd-here-is-how-to-fix-it/

It is true that this is not related to privacy or security, but it is useful (for that, at least). How about moving it to 5000?

@Thorin-Oakenpants
Copy link
Contributor Author

Thorin-Oakenpants commented Dec 6, 2018

my mistake for typing 1023 .. I meant for resume from crash (1022), my bad - will edit title (edit, title was fine) and OP (once) and 2nd post (once). 馃

@claustromaniac
Copy link
Contributor

and my mistake for not paying more attention. But mine is justified because I'm a foreign cat. (yeah, right)

@earthlng
Copy link
Contributor

earthlng commented Dec 9, 2018

The two prefs only control how much to restore, not what is collected in

something that's not stored cannot be restored. The 2 prefs control the number of closed tabs/windows to STORE.

I have tested this, with both set to 0, It still recorded info on multiple tabs and multiple windows

AFAIK open tabs/windows are always stored (at least temporarily) except in PB-only mode. If you set the 2 prefs to 0 then the recorded info gets removed when you close a tab/window.

browser.sessionstore.max_serialize_(back|forward) control the number of per tab back/forward history to store/restore.

@earthlng
Copy link
Contributor

earthlng commented Dec 9, 2018

add info to 0103, and 2803 ... that SR does not work when download & browsing history is cleared on close (see 2803)

good idea to add it to 0103.
IDK if 2803 needs it because people who set FF to restore the session will see it in 0103. Up to you

@Thorin-Oakenpants
Copy link
Contributor Author

I can't wrap my head around this right now "The 2 prefs control the number of closed tabs/windows to STORE" - well that's clear, because it's an undo function.

AFAIK open tabs/windows are always stored (at least temporarily) except in PB-only mode

Yup, got that

If you set the 2 prefs to 0 then the recorded info gets removed when you close a tab/window

OK, that's starting to make more sense to me. In which case it won't impact SR at all (not the restoring open tabs/windows). Maybe it cleans up some history. I will do a test. If it does exactly that, then the prefs are useful, even for those who use session restore, against some persistent local data.

IDK if 2803 needs it because people who set FF to restore the session will see it in 0103

Jesus Christ I need a break. More fucking typos, in OP and elsewhere .. it's 2803 (my bad) Good thinking, also // comments get messy

In the PR open at the moment I also did a change to 2805

  • You do not need these anyway if session restore is disabled (see 1020)
  • becomes => You do not need these anyway if session restore is cleared with history (see 2803)

@earthlng
Copy link
Contributor

earthlng commented Dec 9, 2018

If you set the 2 prefs to 0 then the recorded info gets removed when you close a tab/window

actually it's a bit different for windows, it will always keep the info for the last closed window. That's why you can restore the last closed 2nd window even if the pref is set to 0

@Thorin-Oakenpants
Copy link
Contributor Author

Thorin-Oakenpants commented Dec 10, 2018

OK, so I've been having a play. Never been interested in SR. Something twigged when E said closed, and TBH, it's right in the godamn pref names (undo). By playing around, I getting to know this little beast a bit better.

It seems like all "undo history" is kept in recovery.jsonlz4 (seems like a logical place to keep it). This is not the same as history itself. And as discussed earlier, sanitizing history & downloads includes the whole SR: (recovery, backup recovery, old files). That makes sense.

Tests for max_tabs_undo in one window and over multiple windows do exactly what you think.


FF: nilla 65.0b2
NOTE: Looks like you need to check restore previous on startup, otherwise you only get left with a previous.jsonlz4 all the time ( 馃摎 learnt something today).
PREP: did a few open closes etc and manual wipe everything (ctrl-shift-del) to get rid of files. Also set up 8 websites on the bookmarks toolbar (Thorin is smart, be like Thorin)

test tabs (single window)

  • opened websites 1,2,3,4 then closed 4,3
  • waited 15+ secs to a make sure everything updates (you can see a tmp file write, I'm watching the directory with my pimped out portable Everything)
  • copied file to temp dir
  • closed FF
  • note: same hash for file, can confirm it is not written to on close
  • using a different browser, inspect recovery, it has an "_closedTabs": [ section
  • start FF test profile, restored my two tabs, and undo tab history was there for the 2 closed tabs

now change max_tabs_undo to 0

  • clear everything, repeat test, exactly what you expected, the _closedTabs section is not recorded, and undo tab history is gone on a restart

test tabs (two windows)

  • reset max_tabs_undo, restart
  • opened websites 1,2,3,4 then closed 4,3 in window1
  • opened websites 5,6,7,8 then closed 8,7 in window2
  • closed window2, then window1
  • can see everything in recovery including closed tabs
  • start FF test profile, it opened two windows with 4 tabs and I could undo the 4 closed ones
  • note: closed FF, and it reopened, and it keeps opening two windows. THAT seems incredibly annoying
    • how do you kill a zombie window?
    • I closed all tabs in the zombie 2nd window - OK, that did it ( 馃摎 learnt something MOAR today)

I did not bother to test this by setting max_tabs_undo to zero. It would clearly just not record a _closedTabs section.


So...

  • do I need bother test what happens with max_windows_undo? E's comment makes things messy
  • and then there's what happens in the two cases where one is zero and the other is default, and with E's comment, this whole thing is doing my head in.

@Thorin-Oakenpants
Copy link
Contributor Author

I am beginning to think that these prefs belong under section 0800>history. These are history items, they just happen to be kept in SR.

@Thorin-Oakenpants
Copy link
Contributor Author

Thorin-Oakenpants commented Dec 10, 2018

That's why you can restore the last closed 2nd window even if the pref is set to 0

I had a really good play with this setting. And I am 99.99% sure that the trigger to write to the recovery file is tab activity only. It just happens that each tab is assigned a window as a parent, and split into open and closed. Setting max_windows_undo to 0 doesn't affect the entries written to in the recovery file at all.

note the [SETUP-CHROME] part of 1020: when tested a long time ago, these could NOT affect the recently closed windows, which now makes sense given that only tab activity dictates what is recorded.

schema is something like this

Window 1
   tabs (open ones)
         tab1
              history
              history
              history
         tab2
              history
              history
              history
   _closedTabs
         tab1
              history
              history
              history
Window 2
Window 3
etc

This means Recently Closed Windows history is fully available (as long as there was an open tab in the window when closed - that sounds naff, but see my point on how I managed to kill a zombie window), There's some logic going on, that if the tab is the homepage or newtab, then it doesn't consider it session recoverable. Something like that.

The pref only controls how many windows to restore. I don't know what the code does exactly (eg 1=1 window, 2=2 windows, and 0 is treated as 1 because 1 is the bare minimum).

So to me, these prefs are different

  • max_tabs_undo controls recording undo history and maybe belongs in section 800 - it just happens to record that info in SR.
  • max_windows_undo belongs where it is, but is pretty much useless. It has zero affect on what is recorded. And what's the point of limiting people's SR to one window?

If you want to confirm it. Open a zilla profile etc, enable SR on start, etc. Set max_windows_undo to 0, open 3 windows, with a website in each. Close two windows. Recovery will still have all three windows info in there.

and maybe belongs in section 800 (from just above)

It just depends on how we want to categorize it. Disabling history does not affect session restore. History (places sqlite?) is your actual history, whereas SR is being used for restoring and a set number of undo actions. But SR by its very definition is full of parts of your history (hence why it also gets cleansed when sanitizing).

@earthlng
Copy link
Contributor

0800 is already pretty long. We could just change the header of 1000 to CACHE / SESSION (RE)STORE / FAVICONS

@Thorin-Oakenpants
Copy link
Contributor Author

Thorin-Oakenpants commented Dec 10, 2018

I'm not fussed on where it goes TBH, but I think we could remove max_windows. This allows us to properly explain max_tabs

edit: and just like the naming convention they used, it really is part of "restoring" shit (and MRU stuff has to have a record). It's a different "history" thing altogether. History in 800 (is it places sqlite?) is more about number of visits, frecency, dates/times, top sites, etc.

So yeah, it can stay where it is. Good thinking 馃憤

@Thorin-Oakenpants
Copy link
Contributor Author

modified OP on what to do

@Thorin-Oakenpants
Copy link
Contributor Author

IDK WTF is wrong with me the last few days. Why did I reference adding info to 0103 when it's 0102. That's the third one I've done in this issue - grabbing the subsequent number for some reason.

Thorin-Oakenpants added a commit that referenced this issue Dec 11, 2018
@Thorin-Oakenpants
Copy link
Contributor Author

I don't think we need to add any extra info to resume from crash. Big E said it can be handy if resume from crash keeps crashing (eg a non-lazy-loaded tab is the cause and you're stuck in groundhog day). So rather than remove it, I'll just make it inactive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants