944557 - [Session Restore] Remove the state string from sessionstore-state-write

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

I wonder if we need to keep the notification at all? It doesn't really make sense to notify just before we write to disk as we won't collect any data again and changing the state string isn't possible too.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Well, we have a number of tests using sessionstore-state-write, but I seem to remember that they don't use the status string and that we can replace these with sessionstore-state-write-complete.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Yeah, I wasn't worried about tests, we can always adapt them (mostly) easily.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #1)
> I wonder if we need to keep the notification at all? It doesn't really make
> sense to notify just before we write to disk as we won't collect any data
> again and changing the state string isn't possible too.

Removing the notification would kill the Tor Browser add-on. I'm trying to get in touch with them.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Ugh, yeah we shouldn't do that. Do you know what exactly they're doing with that notification?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Looks like they are just emptying the session string. If this is the only feature we need, we can keep that part easily.

https://mxr.mozilla.org/addons/source/478082/components/tbSessionStore.js#86

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Hmm, yes. OTOH we could probably also provide a way for them to disable sessionstore disk writes altogether. I wonder what gets written to disk, I thought they were using permanent private browsing mode anyway?

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Gk, I have the impression that this part of the add-on is leftover from pre-Private Browsing days. Could you check if the behavior remains intact (i.e. no sessionstore.js written) if you simply don't register the observer?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

As a side-note, even if we want to give clients/add-ons the ability to edit the sessionstore.js before we write it to disk, we can certainly do it in a way that doesn't impact performance if no add-on is registered. It might even be possible to enforce that such rewrites take place only on the worker.

Comment 10

[Georg Koppen]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #8)
> Gk, I have the impression that this part of the add-on is leftover from
> pre-Private Browsing days. Could you check if the behavior remains intact
> (i.e. no sessionstore.js written) if you simply don't register the observer?

It is no leftover. Disabling the observer leaves a sessionstore.js on disc even if I am in PBM. And to the question what gets written to disc if I just disable the code run in the observer, it is something like:

{"windows":[],"selectedWindow":0,"_closedWindows":[],"session":{"state":"running","lastUpdate":1393339612604,"startTime":1393339478980,"recentCrashes":0},"scratchpads":[]}

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

That sounds like we shouldn't keep the notification around only for that? There isn't much value in writing an empty session but we don't leak any information either.

Comment 12

[Georg Koppen]

(In reply to Tim Taubert [:ttaubert] from comment #11)
> That sounds like we shouldn't keep the notification around only for that?
> There isn't much value in writing an empty session but we don't leak any
> information either.

Not sure what you mean with "we don't leak any information either" but if I look at those timestamps above I'd say that is definitely information that gets leaked. And depending on the country a user is living in this alone can be a disaster for her.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Georg Koppen from comment #12)
> Not sure what you mean with "we don't leak any information either" but if I
> look at those timestamps above I'd say that is definitely information that
> gets leaked. And depending on the country a user is living in this alone can
> be a disaster for her.

Ok, then I think we should provide another way of disabling writes to disk altogether. We could of course keep the current notification but I think it would be better to have a more explicit API to do this.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

I'm working on a new API. However, for this specific bug, shouldn't we completely deactivate writes (or even call wipe() upon startup) in permanent private browsing mode?

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

We could just not write a sessionstore.js file if there is no information, we luckily don't need that file for crash detection anymore. With no writes when session.windows.length==0 we would however lose scratchpads if there are any. Especially in this case I think we wouldn't want scratchpads to make us write to disk. The same problem exists for any global values stored in the session.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Don't save sessionstore.js in permanent private browsing mode

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8382114 [details] [diff] [review]
1. Don't save sessionstore.js in permanent private browsing mode

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

r=me with the updateLastSaveTime() call added.

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +196,5 @@
>  
> +    if (PrivateBrowsingUtils.permanentPrivateBrowsing) {
> +      // Don't save in permanent private browsing mode
> +      return Promise.resolve();
> +    }

I think we should still call .updateLastSaveTime() so that we don't collect and try to save state after every invalidation in private browsing mode (which we do in the Tor browser it seems, ugh).

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Removing the string from sessionstore-state-write

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Don't save sessionstore.js in permanent private browsing mode, v2

Applied feedback

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8382169 [details] [diff] [review]
2. Removing the string from sessionstore-state-write

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

::: browser/components/sessionstore/test/browser.ini
@@ -175,5 @@
>  [browser_739531.js]
>  [browser_739805.js]
>  [browser_819510_perwindowpb.js]
>  skip-if = os == "linux" # Intermittent failures, bug 894063
> -[browser_833286_atomic_backup.js]

Why are we removing this here?

::: browser/components/sessionstore/test/head.js
@@ +273,5 @@
>    Services.prefs.setIntPref(PREF, 1000);
>    Services.prefs.setIntPref(PREF, 0);
>    return promise.then(
>      function onSuccess(x) {
> +      Services.prefs.setIntPref(PREF, original);

Why is that better than clearUserPref()?

Comment 21

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #20)
> Comment on attachment 8382169 [details] [diff] [review]
> 2. Removing the string from sessionstore-state-write
> 
> Review of attachment 8382169 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/sessionstore/test/browser.ini
> @@ -175,5 @@
> >  [browser_739531.js]
> >  [browser_739805.js]
> >  [browser_819510_perwindowpb.js]
> >  skip-if = os == "linux" # Intermittent failures, bug 894063
> > -[browser_833286_atomic_backup.js]
> 
> Why are we removing this here?

Ah, right, I got confused with bug 883609, in which I completely rewrote this test as a new test. I'll restore this one.

> ::: browser/components/sessionstore/test/head.js
> @@ +273,5 @@
> >    Services.prefs.setIntPref(PREF, 1000);
> >    Services.prefs.setIntPref(PREF, 0);
> >    return promise.then(
> >      function onSuccess(x) {
> > +      Services.prefs.setIntPref(PREF, original);
> 
> Why is that better than clearUserPref()?

Because one of the tests I have ported actually sets that pref to a specific value.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #21)
> (In reply to Tim Taubert [:ttaubert] from comment #20)
> > > -[browser_833286_atomic_backup.js]
> > 
> > Why are we removing this here?
> 
> Ah, right, I got confused with bug 883609, in which I completely rewrote
> this test as a new test. I'll restore this one.

Thanks.

> > ::: browser/components/sessionstore/test/head.js
> > @@ +273,5 @@
> > >    Services.prefs.setIntPref(PREF, 1000);
> > >    Services.prefs.setIntPref(PREF, 0);
> > >    return promise.then(
> > >      function onSuccess(x) {
> > > +      Services.prefs.setIntPref(PREF, original);
> > 
> > Why is that better than clearUserPref()?
> 
> Because one of the tests I have ported actually sets that pref to a specific
> value.

Ok, that's what I figured. Fine to me, the test is responsible for cleaning up anyway.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Removing the string from sessionstore-state-write, v2

Restored missing test.
Note that I have difficulties getting browser_625016.js to pass, because I don't fully understand it yet.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8382977 [details] [diff] [review]
2. Removing the string from sessionstore-state-write, v2

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

Giving you f+, which means r=me with browser_625016.js passing. We had quite a lot of problems with it in the past (i.e. intermittent failures) and it would be great to get it right.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Removing the string from sessionstore-sate-write, v3

Ok, finally got that last test to pass.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8395659 [details] [diff] [review]
2. Removing the string from sessionstore-sate-write, v3

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

::: browser/components/sessionstore/test/browser_625016.js
@@ +32,5 @@
>  
> +    // Double check that we have no closed windows
> +    is(ss.getClosedWindowCount(), 0, "no closed windows on first save");
> +
> +    newWin.close();

This should use promiseWindowClosed().

@@ +37,5 @@
> +    newWin = null;
> +
> +    let state = JSON.parse((yield promiseSaveFileContents()));
> +    is(state.windows.length, 2,
> +      "observe1: 2 windows in data writted to disk");

*written to disk (that's an ancient typo we can finally fix :)

@@ +39,5 @@
> +    let state = JSON.parse((yield promiseSaveFileContents()));
> +    is(state.windows.length, 2,
> +      "observe1: 2 windows in data writted to disk");
> +    is(state._closedWindows.length, 0,
> +      "observe1: no closed windows in data writted to disk");

*written

@@ +46,5 @@
> +    is(ss.getClosedWindowCount(), 1,
> +      "observe1: 1 closed window according to API");
> +  } finally {
> +    if (newWin) {
> +      newWin.close();

Same here for promiseWindowClosed().

::: browser/components/sessionstore/test/head.js
@@ +85,5 @@
>    });
>  }
> +function promiseWindow(aURL, aFeatures = "") {
> +  let deferred = Promise.defer();
> +  provideWindow(deferred.resolve, aURL, aFeatures);

provideWindow() is only used by really old tests, I would like to see us removing that method in favor of whenNewWindowLoaded(), i.e. something that waits for browser-delayed-startup-finished. We don't need to do this here but at least don't use provideWindow() please.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Removing the string from sessionstore-state-write, v4

Applied feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=75be456bd279

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e10c52688be2
https://hg.mozilla.org/integration/fx-team/rev/b650c8ec24af

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e10c52688be2
https://hg.mozilla.org/mozilla-central/rev/b650c8ec24af

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Waiting for Georg to confirm that we haven't broken Tor Browser in horrible ways.

Comment 31

[Georg Koppen]

Looks good to me, thanks.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Thanks for testing.

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Jorge, do you know of any other addons that use this and might be affected, that QA should be testing? If not, I'll add [qa-] to this and move on without trying to verify it further than Georg already did. Thanks!

Comment 34

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Liz Henry :lizzard from comment #33)
> Hi Jorge, do you know of any other addons that use this and might be
> affected, that QA should be testing? If not, I'll add [qa-] to this and move
> on without trying to verify it further than Georg already did. Thanks!

I only see this one: https://addons.mozilla.org/addon/private-tab/. I'll give the developer a heads up.

Comment 35

[Infocatcher]

(In reply to Jorge Villalobos [:jorgev] from comment #34)
> I only see this one: https://addons.mozilla.org/addon/private-tab/. I'll
> give the developer a heads up.

See https://bugzilla.mozilla.org/show_bug.cgi?id=886447#c3
Now private tabs aren't saved in sessions without any extensions.
And current implementation in Private Tab works fine without session data string (also tested on latest 31.0a1 (2014-04-03)):
https://github.com/Infocatcher/Private_Tab/blob/0.1.7/bootstrap.js#L96-L97
https://github.com/Infocatcher/Private_Tab/blob/0.1.7/bootstrap.js#L1365-L1366

So, all should be fine.

Comment 36

[Infocatcher]

Also now "sessionstore-state-write" notification will be handled only in Gecko < 29:
https://github.com/Infocatcher/Private_Tab/commit/9dddcd3efb0965f581ccffdbc815e038ceb3ddf6
(isn't really needed, but better for performance purposes)

Comment 37

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0

Assured that the private tabs are not saved in session for Private Browsing and using the Private Tab addon.
Tested on latest nightly (Build ID: 20140406030203).
Marking issue verified.