1190801 - Move |_stateSaveSerializer| and the session serialization code to TelemetryStorage

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

All the session serialization code is in TelemetrySession.jsm [1]. We should move it to TelemetryStorage.jsm and ensure we always serialize the write/reads there.

[1] - https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/toolkit/components/telemetry/TelemetrySession.jsm#l1940

Comment 1

[Alessio Placitelli [:Dexter]]

We should also make sure to always serialize the session state on shutdown, not only when official Telemetry is on [1].

[1] - https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/toolkit/components/telemetry/TelemetrySession.jsm#l1833

Comment 2

[Georg Fritzsche [:gfritzsche]]

This is about moving the code from _loadSessionData / _saveSessionData that deals with file operations to TelemetryStorage.

We would expose
* TelemetryStorage.loadSessionData(), returning the data in object form
* TelemetryStorage.saveSessionData(data), receiving the data in object form and saving it as serialized JSON

saveSessionData() should use a SaveSerializer to avoid races in writing to the disk (see the _abortedSessionSerializer usage in TelemetryStorage for an example).

Comment 3

[Georg Fritzsche [:gfritzsche]]

Robert, would you be interested in taking this?

Comment 4

[rthyberg]

yeah. Where specifically should I move _loadSessionData() and _saveSessionData() in telemetryStorage.jsm?
For serializeing the session state on shutdown, would I only need to add logic to check if firefox is shutting down in addition to the previous logic or remove the if statement entirely? 
 e.g. if (Telemetry.isOfficialTelemetry || testing || dectectFirefoxShutdown()) vs remove if(...)

Last, for the save serializer do i just need to call it in a similiar fashion?

Comment 5

[Georg Fritzsche [:gfritzsche]]

(In reply to rthyberg from comment #4)
> yeah. Where specifically should I move _loadSessionData() and
> _saveSessionData() in telemetryStorage.jsm?

You would expose it on TelemetryStorage, and call the actual implementation from there which will be in TelemetryStorageImpl.
See e.g. how loadPendingPing() does it:
https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/toolkit/components/telemetry/TelemetryStorage.jsm#244

> For serializeing the session state on shutdown, would I only need to add
> logic to check if firefox is shutting down in addition to the previous logic
> or remove the if statement entirely? 
>  e.g. if (Telemetry.isOfficialTelemetry || testing ||
> dectectFirefoxShutdown()) vs remove if(...)

We wouldn't need additional shutdown checks really. We are adding a SaveSerializer for this similar to the one for aborted sessions:
https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/toolkit/components/telemetry/TelemetryStorage.jsm#550

This allows us to wait for any pending save tasks on shutdown here:
https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/toolkit/components/telemetry/TelemetryStorage.jsm#589

TelemetryController takes care of calling TelemetryStorage.shutdown(), so we don't need to worry about the details there.

> 
> Last, for the save serializer do i just need to call it in a similiar
> fashion?

Similar to the usage here:
https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/toolkit/components/telemetry/TelemetryStorage.jsm#1529

Basically we keep adding asynchronous "save to file" tasks, that are not started yet.
The SaveSerializer takes care of starting the next queued save task when the previous finished - that way we avoid data races, as with async disk i/o we are never guaranteed the order of multiple competing operations.

Comment 6

[rthyberg]

Attachment: Moved save and load to TelemeteryStorage

Yeah I need some feedback as im kinda lost still. I moved the save and load SessionData to the TelemetryStorageImpl. and I exposed them in TelemeteryStorage. Looking at the other async functions. They seem to have a function like savedArchivedPing() that would call the "_saveArchivedPingTask()". Do i need to do this with  the _saveSessionData(). I would like to give it another shot.

Comment 7

[rthyberg]

Attachment: added_save_load_session_data_to_TeleStorage

same as above just fixed some missing ),

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8666417 [details] [diff] [review]
added_save_load_session_data_to_TeleStorage

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

This looks good so far, that's what i meant :)
I know that this bug is a little less detailed about the steps required then the previous ones; just let me know if i can help with anything.

We can check that things keep working after moving the code to TelemetryStorage by using our unit tests.
The specific test that covers this is test_TelemetrySession.js [0], so a quick way to check your changes is |mach xpcshell-test toolkit/components/telemetry/tests/unit/test_TelemetrySession.js|.

After that works, we can move the |_stateSaveSerializer| over to TelemetryStorage too. I think we'll have to wrap the code in TelemetryStorageImpl.saveSessionData() in another function then, something like:

saveSessionData: function(sessionData) {
  return this._stateSaveSerializer.enqueueTask(() => this._doSaveSessionData(sessionData));
},

_doSaveSessionData: Task.async(function*(sessionData) {
  // ... original save code
},

0: specifically test_savedSessionData(), test_sessionData_ShortSession(), test_invalidSessionData()

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +237,5 @@
> +  /**
> +   * Saves session data to disk
> +   *
> +   */
> +   saveSessionData: function(sessionData) {

Nit: The indention is off a bit here.

@@ +653,5 @@
>  
>    /**
> +   * Saves session data to disk.
> +   */
> +  _saveSessionData: Task.async(function* (sessionData) {

Nit: Lets not prefix this (and loadSessionData()) with "_" - the idea here is to use the "_" prefix for functions only called internally.

@@ +669,5 @@
> +
> +  /**
> +   * Loads session data from the session data file.
> +   * @return {Promise<boolean>} A promise which is resolved with a true argument when
> +   *                            loading has completed, with false otherwise.

This will now have to return the session data as an object (and null on failure).

Comment 9

[rthyberg]

Attachment: Bug-1190801

I got it working for the most part. It is failing one test in test_TelemetrySession.js. This is the result when comparing the subsession count to the expected value. 
"TEST_STATUS: Thread-1 test_savedSessionData FAIL [test_savedSessionData : 1188] 2 == 3787"

Comment 10

[Alessio Placitelli [:Dexter]]

Comment on attachment 8672782 [details] [diff] [review]
Bug-1190801

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

rthyberg, this looks good, I think we're almost there!

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1433,5 @@
>      this._delayedInitTask = new DeferredTask(function* () {
>        try {
>          this._initialized = true;
>  
> +        yield TelemetryStorage.loadSessionData();

This should be changed to |let sessionData = yield TelemetryStorage.loadSessionData();|.

After this, and before |TelemetryStorage.saveSessionData|, you would need to do something like:

this._previousSessionId = sessionData.sessionId;
this._previousSubsessionId = sessionData.subsessionId;
this._profileSubsessionCounter = ...

Check my next comments!

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +243,1 @@
>     },

Even though you didn't do this, but since we're here, would you kindly move the closing "}" a bit to the left, to align it to |saveSessionData|?

@@ +243,5 @@
>     },
>  
> +  /**
> +   * Loads session data from a session data file
> +   *

nit: Empty comment line which could be removed.

@@ +669,5 @@
>  
>    /**
>     * Loads session data from the session data file.
> +   * @return {Promise<Object>} A promise which is resolved with a data object when
> +   *

The next line should really be on this empty comment line.

@@ +705,4 @@
>      }
>  
>      this._previousSessionId = data.sessionId;
>      this._previousSubsessionId = data.subsessionId;

I think we can remove |this._previousSessionId = ...| and |this._previousSessionId = ...|, as those fields are in TelemetrySession, not in TelemetryStorage, and we're returning the |data|.

@@ +708,5 @@
>      this._previousSubsessionId = data.subsessionId;
>      // Add |_subsessionCounter| to the |_profileSubsessionCounter| to account for
>      // new subsession while loading still takes place. This will always be exactly
>      // 1 - the current subsessions.
>      this._profileSubsessionCounter = data.profileSubsessionCounter +

This line (and its continuation on the next line) should be moved out of this function. We should call it in TelemetrySession.jsm, after |yield TelemetryStorage.loadSessionData();| and before |yield TelemetryStorage.saveSessionData(this._getSessionDataObject());|.

This is needed to update the state file with any subsession created before the state file is loaded.

This could be the reason why the test is failing.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8672782 [details] [diff] [review]
Bug-1190801

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

I think Alessio already addressed the important points, i'll only comment on some smaller things i see here.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1322,5 @@
>      if (!Utils.isContentProcess && clearSubsession) {
>        this.startNewSubsession();
>        // Persist session data to disk (don't wait until it completes).
>        let sessionData = this._getSessionDataObject();
> +      this._stateSaveSerializer.enqueueTask(() => TelemetryStorage.saveSessionData(sessionData));

Remember that we moved the _stateSaveSerializer over to the TelemetryStorage module. So now TelemetryStorage is taking care of that part (ensuring those save operations are always run in order, one after each other).

So this line only needs to be |TelemetryStorage.saveSessionData(sessionData);|

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +34,5 @@
>  const DATAREPORTING_DIR = "datareporting";
>  const PINGS_ARCHIVE_DIR = "archived";
>  const ABORTED_SESSION_FILE_NAME = "aborted-session-ping";
>  const DELETION_PING_FILE_NAME = "pending-deletion-ping";
> +const SESSION_STATE_FILE_NAME = "session-state.json";

I think we should also remove this constant from TelemetrySession.jsm.

@@ +678,1 @@
>                                    SESSION_STATE_FILE_NAME);

We can just use |OS.Path.join(gDataReportingDir, SESSION_STATE_FILE_NAME) here.

Comment 12

[rthyberg]

Attachment: bug-1190801

This should be coming to an end pretty soon. I added the save serializer with the help of Dexter. It is still failing one test because it is not counting the subsessions correctly. Wondering if you can take a look to see the last things that need to be done and check any uncaught nits.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8677167 [details] [diff] [review]
bug-1190801

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

I don't see all the changes to _saveSessionData and _loadSessionData in TelemetrySession & TelemetryStorage in this patch.
Is it based on another patch? Can you fold them into one patch?

I can give it a test run and try to find out whats wrong with a complete patch.
From a quick look this looks really close though, thanks.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1320,5 @@
>      if (!Utils.isContentProcess && clearSubsession) {
>        this.startNewSubsession();
>        // Persist session data to disk (don't wait until it completes).
>        let sessionData = this._getSessionDataObject();
> +        TelemetryStorage.saveSessionData(sessionData);

Nit: Please use consistent indentation.

@@ +1433,5 @@
>          this._initialized = true;
>  
> +       let sessionData =  yield TelemetryStorage.loadSessionData();
> +       // Update the session data to keep track of new subsessions created before
> +       // the initialization.

This comments belongs on a line right before the |yield TelemetryStorage.saveSessionData...|.

@@ +1435,5 @@
> +       let sessionData =  yield TelemetryStorage.loadSessionData();
> +       // Update the session data to keep track of new subsessions created before
> +       // the initialization.
> +        if(sessionData) {
> +          this._previousSessionId = sessionData.sessionId;

Lets move the lines here and the |let sessionData = ...| to replace what we currently have in _loadSessionData below, to keep this function more readable.
That will be no big change, only moving the code around a bit.

So we could continue calling |yield this._loadSessionData();| here and _loadSessionData below would become:

_loadSessionData: Task.async(function*() {
  let sessionData = yield TelemetryStorage...
  if (sessionData) {
    ...
  }
}),

Comment 14

[rthyberg]

Attachment: bug-1190801-

For some reason I lost the changes from earlier so I had to remake them.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8681866 [details] [diff] [review]
bug-1190801-

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

This looks really close, thanks for the persistence.

There are two issues remaining here:

(1) This needs to update the members that hold the session data in TelemetrySession.jsm, i commented inline there.
While the code for loading the session data is nearly there, the data in TelemetrySession.jsm is never actually updated after loading.

(2) The test failure, which shows a separate issue:
I ran the failing test specifically, "mach xpcshell-test toolkit/components/telemetry/tests/unit/test_TelemetrySession.js". Right before the actual test failure, there are two errors in the log:

 0:40.08 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) 1446461194398	Toolkit.Telemetry	INFO	TelemetryStorage::_loadSessionData - can not load session data file: ReferenceError: assignment to undeclared variable content (resource://gre/modules/TelemetryStorage.jsm:736:23) JS Stack trace: TelemetryStorageImpl._loadSessionData<@TelemetryStorage.jsm:736:23 < TelemetryStorageImpl.loadSessionData/<@TelemetryStorage.jsm:729:56 < [e-mail]:495:17 < [e-mail]:466:7 < [e-mail]:729:12 < [e-mail]:245:12 < setupChromeProcess/this._delayedInitTask<@TelemetrySession.jsm:1443:15 < [e-mail]:314:40 < [e-mail]:275:3 < createAsyncFunction/[e-mail]:249:14 < [e-mail]:164:12 < this.DeferredTask.prototype._timerCallback/<@DeferredTask.jsm:282:13 < [e-mail]:330:41 < [e-mail]:275:3 < createAsyncFunction/[e-mail]:249:14 < [e-mail]:164:12 < [e-mail]:280:29 < [e-mail]:208:5 < [e-mail]:519:5 < @-e:1:1"
 0:40.08 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) 1446461194419	Toolkit.Telemetry	TRACE	TelemetryStorage::_popAndPerformQueuedOperation - Performing queued operation."
 0:40.18 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "1446461194489	Toolkit.Telemetry	ERROR	TelemetryStorage::_saveSessionData - Failed to write session data to /var/folders/tp/mgx9wlds46j6q3ck3xhjzm2w0000gn/T/firefox/xpcshellprofile/datareporting/session-state.json: ReferenceError: CommonUtils is not defined (resource://gre/modules/TelemetryStorage.jsm:715:7) JS Stack trace: TelemetryStorageImpl._saveSessionData<@TelemetryStorage.jsm:715:7 < TelemetryStorageImpl.saveSessionData/<@TelemetryStorage.jsm:706:58 < [e-mail]:495:17 < [e-mail]:466:7 < [e-mail]:706:14 < [e-mail]:237:13 < setupChromeProcess/this._delayedInitTask<@TelemetrySession.jsm:1447:15 < [e-mail]:314:40 < [e-mail]:934:23 < [e-mail]:813:7 < Promise*[e-mail]:744:11 < [e-mail]:776:7 < [e-mail]:711:7 < onSuccess@osfile_native.jsm:63:7 < [e-mail]:208:5 < [e-mail]:519:5 < @-e:1:1" {file: "resource://gre/modules/Log.jsm" line: 751}]"
 0:40.21 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) 1446461194508	Toolkit.Telemetry	TRACE	TelemetrySession::gatherMemory"
 0:40.26 TEST_STATUS: Thread-1 test_savedSessionData FAIL [test_savedSessionData : 1161] 0 == 1
    /Users/gfritzsche/moz/mc1/obj/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js:test_savedSessionData:1161
    _run_next_test@/Users/gfritzsche/moz/mc1/testing/xpcshell/head.js:1467:9
    do_execute_soon/<.run@/Users/gfritzsche/moz/mc1/testing/xpcshell/head.js:659:9
    _do_main@/Users/gfritzsche/moz/mc1/testing/xpcshell/head.js:208:5
    _execute_test@/Users/gfritzsche/moz/mc1/testing/xpcshell/head.js:519:5
    @-e:1:1


The first error points to TelemetryStorage.jsm:736 and mentions "assignment to undeclared variable content". And indeed, that function is missing a declaration for |content| like |let content;| before the try-catch.

The second error mentions "ReferenceError: CommonUtils is not defined" for TelemetryStorage.jsm - we need to import CommonUtils there like TelemetrySession already does:
https://dxr.mozilla.org/mozilla-central/rev/96377bdbcdf3e444a22aeaa677da696243b00d98/toolkit/components/telemetry/TelemetrySession.jsm#145

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ -1445,2 @@
>  
> -        yield this._loadSessionData();

Lets continue to use |yield this._loadSessionData();| here.

@@ -1861,5 @@
> -   * Loads session data from the session data file.
> -   * @return {Promise<boolean>} A promise which is resolved with a true argument when
> -   *                            loading has completed, with false otherwise.
> -   */
> -  _loadSessionData: Task.async(function* () {

I think we should not remove this function.
Let's instead remove the contents up until...

@@ -1882,5 @@
> -      this._log.error("_loadSessionData - failed to parse session data", ex);
> -      Telemetry.getHistogramById("TELEMETRY_SESSIONDATA_FAILED_PARSE").add(1);
> -      return false;
> -    }
> -

... here.
Now, the rest of the function should just keep working fine if we add this line here:

let data = yield TelemetryStorage.loadSessionData();

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +748,5 @@
> +      Telemetry.getHistogramById("TELEMETRY_SESSIONDATA_FAILED_PARSE").add(1);
> +      return null;
> +    }
> +
> +    if (!data ||

I think the rest of this function belongs into the TelemetrySession module (this module is about the file handling details, TelemetrySession is about the session details).
Lets just return |data| here and let TelemetrySession handle the details as commented.

Comment 16

[rthyberg]

Attachment: bug-1190801-

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8683011 [details] [diff] [review]
bug-1190801-

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

This looks good! All tests should pass if you take care of the _loadSessionData function thing.

The other suggestions are minor and mostly about the style [0].

[0] - https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1439,5 @@
>      this._delayedInitTaskDeferred = Promise.defer();
>      this._delayedInitTask = new DeferredTask(function* () {
>        try {
>          this._initialized = true;
> +        yield this._loadSessionData();

nit: don't move this line

@@ +1859,1 @@
>     * @return {Promise<boolean>} A promise which is resolved with a true argument when

nit: we should update this comment

@@ +1859,5 @@
>     * @return {Promise<boolean>} A promise which is resolved with a true argument when
>     *                            loading has completed, with false otherwise.
>     */
>    _loadSessionData: Task.async(function* () {
> +   let data = yield TelemetryStorage.loadSessionData();

We should bail out early here if data is null, as we were not able to load the file. Otherwise, we would be incrementing the TELEMETRY_SESSIONDATA_FAILED_VALIDATION histogram below, which is wrong. So:

if (!data) {
  return null;
}

@@ +1860,5 @@
>     *                            loading has completed, with false otherwise.
>     */
>    _loadSessionData: Task.async(function* () {
> +   let data = yield TelemetryStorage.loadSessionData();
> +   if (!data || !("profileSubsessionCounter" in data) ||

Since we added the previous if, we should remove the |!data || | part from this if.

@@ +1870,3 @@
>      }
>  
> +   if(data) {

We can remove the if(data) {} from here, as we're checking this earlier.

Leave the 3 this._* lines, just don't protect them with the if.

@@ +1879,1 @@
>    }),

Nit: don't remove this empty line.

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +46,5 @@
>  XPCOMUtils.defineLazyGetter(this, "gDeletionPingFilePath", function() {
>    return OS.Path.join(gDataReportingDir, DELETION_PING_FILE_NAME);
>  });
> +XPCOMUtils.defineLazyModuleGetter(this, "CommonUtils",
> +                                  "resource://services-common/utils.js");

nit: keep the empty line below this one

@@ +231,5 @@
>    },
>  
>    /**
> +   * Saves session data to disk
> +   *

nit: remove the "*" on the line with no text.

@@ +239,5 @@
> +  },
> +
> + /**
> +  * Loads session data from a session data file
> +  *

nit: same here.

@@ +701,5 @@
>  
>    /**
> +   * Saves session data to disk.
> +   */
> +

Nit: Remove the empty line between the comment and the function.

@@ +724,5 @@
> +   * Loads session data from the session data file.
> +   * @return {Promise<boolean>} A promise which is resolved with a true argument when
> +   *                            loading has completed, with false otherwise.
> +   */
> +

Nit: Remove the empty line between the comment and the function.

Comment 18

[rthyberg]

Attachment: bug-1190801-

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8683031 [details] [diff] [review]
bug-1190801-

Bouncing the review to Georg!

Comment 20

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8683031 [details] [diff] [review]
bug-1190801-

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

Nice, this looks good now!
Moving on to only some style issues and code cleanup.

For cleanup:
* Now we don't need the SaveSerializer code anymore in TelemetrySession.jsm, lets remove it.
* CommonUtils isn't used anymore in TelemetrySession.jsm, lets remove it

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1861,3 @@
>     */
>    _loadSessionData: Task.async(function* () {
> +   let data = yield TelemetryStorage.loadSessionData();

Nit: The indentation for this functions contents is off.
We should use 2 spaces per indentation level.

@@ +1864,2 @@
>  
> +   if(!data) {

Nit: space before the opening paranthesis.

@@ +1876,5 @@
>  
> +   this._previousSessionId = data.sessionId;
> +   this._previousSubsessionId = data.subsessionId;
> +   this._profileSubsessionCounter = data.profileSubsessionCounter
> +                                                 + this._subsessionCounter;

Nit: Formatting is off here - we should have the "+" on the previous line and the "this._subsessionCounter;" lined up with the "data." part.

@@ +1877,5 @@
> +   this._previousSessionId = data.sessionId;
> +   this._previousSubsessionId = data.subsessionId;
> +   this._profileSubsessionCounter = data.profileSubsessionCounter
> +                                                 + this._subsessionCounter;
> +   return data

Missing semicolon after the statement.

@@ -1896,5 @@
> -    this._previousSessionId = data.sessionId;
> -    this._previousSubsessionId = data.subsessionId;
> -    // Add |_subsessionCounter| to the |_profileSubsessionCounter| to account for
> -    // new subsession while loading still takes place. This will always be exactly
> -    // 1 - the current subsessions.

We should not remove this comment.

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +230,5 @@
>      return TelemetryStorageImpl.savePendingPing(ping);
>    },
>  
> + /**
> +  * Saves session data to disk

Can you add @param ("The session data.") and @return ("{Promise} Resolved when the data was saved.") descriptions here?
Nit: indentation is off by one space.

@@ +233,5 @@
> + /**
> +  * Saves session data to disk
> +  */
> +  saveSessionData: function(sessionData) {
> +     return TelemetryStorageImpl.saveSessionData(sessionData);

Nit: indentation is one space too much.

@@ +237,5 @@
> +     return TelemetryStorageImpl.saveSessionData(sessionData);
> +  },
> +
> + /**
> +  * Loads session data from a session data file

Similar @return documentation here ("{Promise<object>} Resolved with the session data in object form.").
Nit: indentation is off by one space.

@@ +700,5 @@
>    /**
> +   * Saves session data to disk.
> +   */
> +  saveSessionData: function(sessionData) {
> +      return this._stateSaveSerializer.enqueueTask(() => this._saveSessionData(sessionData));

More indentation issues here and below, lets take a closer look and fix them.

@@ +719,5 @@
> +
> +  /**
> +   * Loads session data from the session data file.
> +   * @return {Promise<boolean>} A promise which is resolved with a true argument when
> +   *                            loading has completed, with false otherwise.

Nit: We can simplify this to "Resolved with true when ..."

@@ +745,5 @@
> +      this._log.error("_loadSessionData - failed to parse session data", ex);
> +      Telemetry.getHistogramById("TELEMETRY_SESSIONDATA_FAILED_PARSE").add(1);
> +      return null;
> +    }
> +    return data;

Nit: Lets add a blank line before the return statements.

Comment 21

[rthyberg]

Attachment: bug-1190801-

I ended up changing one of the promises to an Object because we werereturning an object in TelemetryStorage._loadSessionData.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8683302 [details] [diff] [review]
bug-1190801-

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

Some smaller nits remain here, nearly there :)
Also, this is still missing these two cleanup points:
* We don't need the SaveSerializer code anymore in TelemetrySession.jsm, lets remove it:
  https://dxr.mozilla.org/mozilla-central/rev/6077f51254c69a1e14e1b61acba4af451bf1783e/toolkit/components/telemetry/TelemetrySession.jsm#266-355
* CommonUtils isn't used anymore in TelemetrySession.jsm, lets remove it:
  https://dxr.mozilla.org/mozilla-central/rev/6077f51254c69a1e14e1b61acba4af451bf1783e/toolkit/components/telemetry/TelemetrySession.jsm#145-146

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1864,3 @@
>  
> +    if (!data) {
> +       return null

Nit: Missing semicolon at the end of this line, indented one space too far.

@@ +1871,5 @@
>          !("subsessionId" in data) || !("sessionId" in data)) {
> +       this._log.error("_loadSessionData - session data is invalid");
> +       Telemetry.getHistogramById("TELEMETRY_SESSIONDATA_FAILED_VALIDATION").add(1);
> +       return null;
> +     }

Nit: The previous 4 lines are indented one space too far.

@@ +1878,5 @@
>      this._previousSubsessionId = data.subsessionId;
>      // Add |_subsessionCounter| to the |_profileSubsessionCounter| to account for
>      // new subsession while loading still takes place. This will always be exactly
>      // 1 - the current subsessions.
> +    this._profileSubsessionCounter = data.profileSubsessionCounteri +

A stray "i" came in here - this should be ".profileSubsessionCounter +".

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +235,5 @@
> +   * @param ("The session data")
> +   * @return {Promise} Resolved when the data was saved.
> +   */
> +  saveSessionData: function(sessionData) {
> +     return TelemetryStorageImpl.saveSessionData(sessionData);

Nit: This doesn't use 2-spaces indentation (5 leading spaces instead of 4).

@@ +704,5 @@
> +   * Saves session data to disk.
> +   */
> +  saveSessionData: function(sessionData) {
> +    return this._stateSaveSerializer.enqueueTask(() => this._saveSessionData(sessionData));
> +   },

Nit: Indented one space too far.

Comment 23

[rthyberg]

Attachment: bug-1190801-

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8684027 [details] [diff] [review]
bug-1190801-

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

Nice, i think this is done, i am pushing it to try to see that it works fine across the platforms.
Remaining asks:
* Fix the small nits mentioned below
* A slightly improved commit message, e.g.: "Bug 1190801 - Move session state serialization code to Telemetry storage. r=gfritzsche"

Thanks!

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +231,5 @@
>    },
>  
>    /**
> +   * Saves session data to disk.
> +   * @param ("The session data")

@param {Object} sessionData The session data.

@@ +240,5 @@
> +  },
> +
> +  /**
> +   * Loads session data from a session data file.
> +   * {Promise<object>} Resolved with the session data in object form.

@return {Promise<object>} Resolved with the session data in object form.

@@ +722,5 @@
> +
> +  /**
> +   * Loads session data from the session data file.
> +   * @return {Promise<Object>} A promise which is resolved when
> +   *                            loading has completed, with null otherwise.

A promise resolved with an object on success, with null otherwise.

Comment 25

[Georg Fritzsche [:gfritzsche]]

Pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=134ce86544d9

Comment 26

[rthyberg]

Attachment: bug-1190801-

Comment 27

[Georg Fritzsche [:gfritzsche]]

I don't see any related failures, so this is good to go.

Thanks for the persistence Robert, i realize this bug was a little more involved than i planned it to be :)

Comment 28

[Pulsebot]

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

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8c7a7e0e216