1804374 - Imap autosync may no longer occur after wake-up from hibernate, sleep or after offline to online

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[gene smith]

+++ This bug was initially created as a clone of Bug #1776823 +++

While troubleshooting bug 1802295 I noticed that after the computer woke up from hibernation that autosync sometimes didn't occur (see bug 1802295 comment 25 and other comments below that). At first I thought it was something about the elapsed time determination but concluded that was OK. Also, the bug is hard to duplicate and have only seen it may 2 or 3 times in the last 1.5 weeks and after attaching GDB debugger I could see that the nsImapAutoSyncManager::mUpdateState, an enum, had the value initiated but it needed the value completed for autosync to occur again. It's set to completed only in the onStopRunningUrl() or at startup. If it is stuck at initiated it it won't trigger another update URL so onStopRunningUrl will never occur resulting in no autosync occurring until restart.

I finally realized, after seeing autosync stop and then attaching GDB another time, that hibernate or sleep or going offline causes an autosync Pause() and on wake up or going online an autosync function Resume(), both driven by the autosync Observe(). So what's happening is that rarely hibernation (or sleep or going offline) occurs right after mUpdateState is set to initiated but the resulting URL (folderstatus) never finishes (due to timeout during hibernation or offline) so onStopRunningUrl() never occurs to set mUpdateState back to completed, thus locking up autosync.

My fix is to effectively set mUpdateState to completed in Resume(). However, I changed the enum mUpdateState having values completed and initiated to just a bool called mUpdateInProgress where false is equivalent to completed and true is equivalent to initiated. I personally find this naming easier to understand rather than a 2-value enum.

Note: I couldn't find any documentation, comments or bug reports mentioning mUpdateState but it appears to cause the folder updates to occur in series rather than overlapping so that folder B update doesn't occur until folder A update finishes. So I suppose this keeps autosync from using as much CPU at a time.

I made a few comment and logging changes that don't affect functionality. Also, when the folder status check finds nothing needing to be downloaded, I'm also setting mUpdateInProgress false. This was my first attempt at a fix but after over a week I haven't actually seen this occur but I left it in place just in case.

Comment 1

[gene smith]

Attachment: Bug 1804374 - Imap autosync sometimes stops on sleep/hibernate wake-up. r=BenC

Comment 2

[gene smith]

Attachment: Bug 1804374 - Imap autosync sometimes stops on sleep/hibernate wake-up. r=BenC

Comment 3

[gene smith]

Comment on attachment 9307001 [details]
Bug 1804374 - Imap autosync sometimes stops on sleep/hibernate wake-up. r=BenC

This failed try build for 32-bit due a wrong format specifier.

Comment 4

[Wayne Mery (:wsmwk)]

Good to land before Monday?

Comment 5

[gene smith]

There's a clang-format complaint in my submitted patch that I need to fix. Will fix it next.
Also, I changed the summary to better describe the bug.

Edit:
Also, would have done this sooner but never received the usual email from "moz-phab" telling me the review was completed.

Comment 6

[gene smith]

Attachment: Bug 1804374 - Imap autosync sometimes stops on sleep/hibernate wake-up. r=BenC

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9d7b383aee62
Imap autosync sometimes stops on sleep/hibernate wake-up. r=BenC

Comment 8

[gene smith]

Comment on attachment 9307010 [details]
Bug 1804374 - Imap autosync sometimes stops on sleep/hibernate wake-up. r=BenC

It looks this got pushed instead of the the newer version with the fix for clang-format.

Comment 9

[Alessandro Castellani [:aleca]]

Does it need a backout?

Comment 10

[gene smith]

(In reply to Alessandro Castellani [:aleca] from comment #9)

Does it need a backout?

Only if a clang-format error causes a bustage. I don't know if it does.

Comment 11

[Alessandro Castellani [:aleca]]

Do you want to create a quick follow up patch to fix the wrong push?
Also, you shouldn't mark a patch that landed as obsolete unless it gets backed out, otherwise it messes up the flags and the reliability of the bug history.

Comment 12

[Andrei [:sancus]]

(In reply to gene smith from comment #8)

It looks this got pushed instead of the the newer version with the fix for clang-format.

Adding checkin-needed-tb when the right patch hasn't been approved yet is going to cause this type of confusion. In the future I'd triple check that the patch you want checked in is approved and that it's 100% clear which one needs check in.

Comment 13

[gene smith]

Attachment: Bug 1804374 - Fix for clang-format. r=aleca

Comment 14

[gene smith]

(In reply to Alessandro Castellani [:aleca] from comment #11)

Do you want to create a quick follow up patch to fix the wrong push?

It's on the way...

Comment 15

[gene smith]

(In reply to Andrei Hajdukewycz [:sancus] from comment #12)

(In reply to gene smith from comment #8)

It looks this got pushed instead of the the newer version with the fix for clang-format.

Adding checkin-needed-tb when the right patch hasn't been approved yet is going to cause this type of confusion. In the future I'd triple check that the patch you want checked in is approved and that it's 100% clear which one needs check in.

I thought I marked the original patch (which was approved by BenC) obsolete before I sent today, via moz-phab a revised patch having the clang-format fixed. So I assumed "checkin-needed" applied to the newest one I sent.
Also, would have taken care of this earlier but for some reason I never got an email telling me the patch was reviewed several days ago.

Comment 16

[Alessandro Castellani [:aleca]]

Thanks for the follow-up, I'll land it now.

Are the other Phab patches in this bug needing to be reviewed?
Was the patch that landed correct except for the code formatting?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2559fa63c9e3
Fix for clang-format. r=aleca

Comment 18

[gene smith]

(In reply to Alessandro Castellani [:aleca] from comment #16)

Thanks for the follow-up, I'll land it now.

Are the other Phab patches in this bug needing to be reviewed?

I don't think so. See below

Was the patch that landed correct except for the code formatting?

Yes.

Ben reviewed and approved the patch on Wed. but it had the clang-format error still in it.
Wayne commented today in this bug about wanting this landed before Monday.
I then looked at the patch and noticed Ben had approved it but I didn't get an email notification of that.
I then amended my commit with just the clang-format fix and submitted it via moz-phab
Then I marked the one Ben approved obsolete and then set the checkin-needed-tb keyword

Anyhow, on a previous bug I was told by Magnus that a minor change like the 1 or 2 line clang-format fix didn't need another approval by BenC so I don't know for sure if more review is needed. But, I don't think so, based on what Magnus told me before.

Comment 19

[Alessandro Castellani [:aleca]]

Anyhow, on a previous bug I was told by Magnus that a minor change like the 1 or 2 line clang-format fix didn't need another approval by BenC so I don't know for sure if more review is needed. But, I don't think so, based on what Magnus told me before.

Indeed, a simple code style formatting doesn't need an extra review.
Thanks for doing the follow up, I already pushed it so it's all good.
You can close and abandon that extra Phab patch still in Needs revision, so the bug then it's correct.

Comment 20

[gene smith]

Comment on attachment 9307627 [details]
Bug 1804374 - Imap autosync sometimes stops on sleep/hibernate wake-up. r=BenC

Didn't need this after all. Clang-format issue fixed in separate patch.