1034075 - [Session Restore] SessionFile blocker should offer information on whether a write has been completed

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

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

It is useful for debugging to know whether at least one write has been completed. We should add this to the state of the SessionFile AsyncShutdown blocker.

Comment 1

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

Attachment: More details in case of SessionWorker AsyncShutdown timeout

Let's take the opportunity to add a few more details.

Comment 2

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

Comment on attachment 8451298 [details] [diff] [review]
More details in case of SessionWorker AsyncShutdown timeout

Ah Tim is apparently on PTO.

Comment 3

[Steven MacLeod [:smacleod]]

Comment on attachment 8451298 [details] [diff] [review]
More details in case of SessionWorker AsyncShutdown timeout

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

Sorry for the delay in review, I was mistakenly assuming some of the feedback
from when you were trying to uplift this stuff would apply here. My bad!

This looks fine to me.

Just an aside though, I was reading inside AsyncShutdown.jsm when reviewing
this patched and noticed there is mixed indentation with tabs and spaces[1].
I haven't checked this patch to make sure it's only using space characters
for indentation, so you should probably check that your editor is configured
correctly.


[1] http://hg.mozilla.org/mozilla-central/diff/993e88c0c67e/toolkit/modules/AsyncShutdown.jsm#l1.154

::: browser/components/sessionstore/src/SessionFile.jsm
@@ +298,5 @@
> +        Promise.reject(Object.create(ex, {
> +          message: {
> +            value: "Could not write session state file " + ex
> +          }
> +        })); // Make sure that errors are reported and cause test failures

This seems like a strange, roundabout way to accomplish this. Is it really the only way to report and cause a test failure?

::: browser/components/sessionstore/src/SessionWorker.jsm
@@ +41,5 @@
> + */
> +let gDiagnostics = {
> +  /**
> +   * `true` if at least one operation was completed, whether this was a success
> +   * or a failure.

", whether this was a success or a failure" -> ", including failed operations".

@@ +51,5 @@
> +   */
> +  hasEverSucceeded: false,
> +
> +  /**
> +   * `true` if at least one of the operations has failed because of an

"one of the operations" - > "one operation"

Comment 4

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

(In reply to Steven MacLeod [:smacleod] from comment #3)
> Comment on attachment 8451298 [details] [diff] [review]
> More details in case of SessionWorker AsyncShutdown timeout
> 
> Review of attachment 8451298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay in review, I was mistakenly assuming some of the feedback
> from when you were trying to uplift this stuff would apply here. My bad!
> 
> This looks fine to me.
> 
> Just an aside though, I was reading inside AsyncShutdown.jsm when reviewing
> this patched and noticed there is mixed indentation with tabs and spaces[1].
> I haven't checked this patch to make sure it's only using space characters
> for indentation, so you should probably check that your editor is configured
> correctly.

Ah, thanks.
>
> ::: browser/components/sessionstore/src/SessionFile.jsm
> @@ +298,5 @@
> > +        Promise.reject(Object.create(ex, {
> > +          message: {
> > +            value: "Could not write session state file " + ex
> > +          }
> > +        })); // Make sure that errors are reported and cause test failures
> 
> This seems like a strange, roundabout way to accomplish this. Is it really
> the only way to report and cause a test failure?

I can think of several variants, but they all revolve around using `Promise.reject(someException)`. Now, instead of `Object.create`, I could create a new error, copy the stack & co and add a new error message.

Comment 5

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

Attachment: MozReview Request: bz://1034075/Yoric

/r/9033 - Bug 1034075 - Add more details to SessionFile AsyncShutdown blocker;r=ttaubert

Pull down this commit:

hg pull -r a2c7928910eab014f351f96ff2353325f0339f1f https://reviewboard-hg.mozilla.org/gecko/

Comment 6

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

Comment on attachment 8607479 [details]
MozReview Request: bz://1034075/Yoric

/r/9033 - Bug 1034075 - Add more details to SessionFile AsyncShutdown blocker;r=ttaubert

Pull down this commit:

hg pull -r a2c7928910eab014f351f96ff2353325f0339f1f https://reviewboard-hg.mozilla.org/gecko/

Comment 7

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

Comment on attachment 8607479 [details]
MozReview Request: bz://1034075/Yoric

/r/9033 - Bug 1034075 - Add more details to SessionFile AsyncShutdown blocker;r=ttaubert

Pull down this commit:

hg pull -r a2c7928910eab014f351f96ff2353325f0339f1f https://reviewboard-hg.mozilla.org/gecko/

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://reviewboard.mozilla.org/r/9033/#review7637

Ship It!

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8607479 [details]
MozReview Request: bz://1034075/Yoric

I am confused.

Comment 10

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

Sorry about that, I uploaded an incomplete patch.

Comment 11

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

Comment on attachment 8607479 [details]
MozReview Request: bz://1034075/Yoric

/r/9033 - Bug 1034075 - Add more details to SessionFile AsyncShutdown blocker;r=ttaubert

Pull down this commit:

hg pull -r 6796e3371c4907dde26f1bbda8f7c7672d308dce https://reviewboard-hg.mozilla.org/gecko/

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8607479 [details]
MozReview Request: bz://1034075/Yoric

https://reviewboard.mozilla.org/r/9031/#review7641

Ship It!

Comment 13

[Pulsebot]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ec340de98e1

Comment 16

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

Attachment: MozReview Request: Bug 1034075 - Add more details to SessionFile AsyncShutdown blocker;r=ttaubert