904477 - Add missing error handlers for promises used in SessionStore

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

As reported in bug 902866 we are missing a couple of error handlers when using promises, let's fix that. Here's what we need to do:

--> SessionStore.jsm
 + SessionStore.init() calls gSessionStartup.onceInitialized.then()
   http://hg.mozilla.org/mozilla-central/file/3d20597e0a07/browser/components/sessionstore/src/SessionStore.jsm#l390
 + SessionStore._saveStateObject() calls _SessionFile.write()
   http://hg.mozilla.org/mozilla-central/file/3d20597e0a07/browser/components/sessionstore/src/SessionStore.jsm#l3775

--> nsSessionStartup.js
 + nsSessionStartup.init() calls _SessionFile.read()
   http://hg.mozilla.org/mozilla-central/file/3d20597e0a07/browser/components/sessionstore/src/nsSessionStartup.js#l83


In order to fix this we need to pass Cu.reportError to .then() to convert calls like this:

> _SessionFile.read(() => next());

to this:

> _SessionFile.read(() => next(), Cu.reportError);

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Awww, shoot. The examples at the bottom should have been:

> _SessionFile.read().then(() => next());

and:

> _SessionFile.read().then(() => next(), Cu.reportError);

Comment 2

[Brian Graham]

Is this issue obsoleted? Seems like an easy first bug to fix.

Comment 3

[AKSHAY NAUTIYAL]

i am interested to solve this as my first bug.How do i go about this thanks in advance.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Sorry for the late response here, this slipped off my radar. Thanks for your interest in fixing this however!

comment #0 describes the three files that need to be fixed and what needs to be done. If any of you wants to take this I can assign the bug to you.

Comment 5

[Brian Graham]

IPlease assign the ticket to me and I will try to fix this tonight between 500 UTC and 1000 UTC as my first fix.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

No hurry! It's yours now :)

Comment 7

[Brian Graham]

Attachment: Bug904477.patch

Comment 8

[Brian Graham]

Some of the areas you had mentioned changed since the report, so I grepped the directory for `.then(` calls and added `Cu.reportError` everywhere a `function onError` wasn't already provided. Some calls were not using using arrow functions as in the example, I'm mostly confident this is fine but if it's explicitly just those instances I can re-patch this.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 796388 [details] [diff] [review]
Bug904477.patch

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

Thank you and sorry for the changes that happened in between. This looks good if we remove the one thing mentioned below.

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +205,5 @@
>    read: function () {
>      return SessionWorker.post("read").then(msg => {
>        this._recordTelemetry(msg.telemetry);
>        return msg.ok;
> +    }, Cu.reportError);

No need to add an error handler here, we expect the caller to do that. See your change to nsSessionStartup.js.

Comment 10

[Brian Graham]

Attachment: Bug904477.v2.patch -- Without error handler on SessionWorker.post("read").

Ah. The new patch is attached.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 802627 [details] [diff] [review]
Bug904477.v2.patch -- Without error handler on SessionWorker.post("read").

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

Thanks a lot, Brian! Sorry for the review delay, I was out for a couple of days.

Can you please prepare your patch for checkin-needed and mark it as such?

Comment 12

[Brian Graham]

Comment on attachment 802627 [details] [diff] [review]
Bug904477.v2.patch -- Without error handler on SessionWorker.post("read").

I believe this patch follows all the steps on the wiki[1] for checking in a patch. Let me know if this isn't the correct format and I will revise it.

Thanks!


[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F unless

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Brian, the patch looks good but it's missing a tiny thing. Can you please add the reviewer's handle to the end of the patch description? Like so:

Bug 904477 - Add missing error handlers for promises used in SessionStore r=ttaubert

Thanks!

Comment 14

[Brian Graham]

Attachment: Bug904477.reviewed.patch

Now with r=ttaubert in commit message.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 805940 [details] [diff] [review]
Bug904477.reviewed.patch

Thanks for the patch, Brian! In the future, you can just use the checkin-needed bug keyword for your request. The checkin? flag is more intended for situations where there are multiple patches that aren't all landing at the same time.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Brian Graham]

Thanks Tim and Ryan. I'll take better note of the process here for the next one!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Brian Graham from comment #17)
> Thanks Tim and Ryan. I'll take better note of the process here for the next
> one!

No worries, the process is something we all had to get used to :) Thank you for your contribution!