Last Comment Bug 682244 - Auto-Update of CRLs not working with DD.MM.YYYY date locale
: Auto-Update of CRLs not working with DD.MM.YYYY date locale
Status: RESOLVED FIXED
[sg:moderate]
:
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla14
Assigned To: Steve Workman [:sworkman] (please use needinfo)
:
Mentors:
: 645555 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-26 03:24 PDT by bjoern
Modified: 2012-03-26 10:46 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1.0 patch (2.04 KB, patch)
2012-01-24 17:05 PST, Steve Workman [:sworkman] (please use needinfo)
no flags Details | Diff | Review
v1.1 Fixed Diff format (2.07 KB, patch)
2012-02-15 15:19 PST, Steve Workman [:sworkman] (please use needinfo)
brian: superreview-
Details | Diff | Review
v2.0 removed PRFormatTime (5.44 KB, patch)
2012-03-03 17:35 PST, Steve Workman [:sworkman] (please use needinfo)
no flags Details | Diff | Review
v2.1 Minor build fixes (5.46 KB, patch)
2012-03-03 17:39 PST, Steve Workman [:sworkman] (please use needinfo)
no flags Details | Diff | Review
v2.2 Deal with time string to integer = 0 (6.26 KB, patch)
2012-03-21 12:41 PDT, Steve Workman [:sworkman] (please use needinfo)
kaie: review+
brian: superreview+
Details | Diff | Review

Description bjoern 2011-08-26 03:24:34 PDT
User Agent: Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 2011080400

Steps to reproduce:

CLRs are (still) not being updated, which is quite security critical.

More details can be found in bug #371522 but that bug is close fixed and the status is not changable. There is also no reaction from Mozilla developers there about the latest comments, that's why I have to open this new bug report.
Comment 1 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-08-26 06:51:39 PDT
This was something which was resolved over 2 years ago. If this is now a problem for you in Firefox 6, this is a regression. Please post steps to reproduce, a clearer description of the problem you are seeing, and provide a regression range if possible.

Thanks
Comment 2 bjoern 2011-08-26 07:25:07 PDT
the point is that the reported problem was probably not corrected. Configure whatever CRL you want, wait for the day before the schedules auto-update day and see what happens. Nothing will happend unfortunately like you can see in the screenshot (https://bugzilla.mozilla.org/show_bug.cgi?id=371522#c23). You can also use the CRL URL that you see there to reproduce the problem. I'm not repeating all of the bug descriptions from bug #371522 because *this* bug is just there because #371522 cannot be reopened by ordinary bugzilla users.
Comment 3 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-08-26 07:31:33 PDT
Work will not continue on that bug because it is has been fixed long ago. I implore you to pull over pertinent information from that bug or to simply provide the required information to reproduce. We also need a regression window.

Without that information in this bug, it will never be resolved.
Comment 4 Christian Holler (:decoder) 2011-08-29 05:17:55 PDT
I can confirm this partially (Gecko/20110828 Firefox 8.0a2): There seem's to be an off-by-one in the "x days before next update" function. I imported a CRL that has next update field:

Next Update: Sep  6 21:03:09 2011 GMT

Now I enable auto-update and specify "Update 1 day(s) before Next Update date". I'd expect this to update at Sep  5 21:03:09 2011 GMT, but it does not, it updates at Sep  6. I deleted the CRL, reimported and set auto-update to "Update 5 day(s) before Next Update date". As expected, it does NOT update at Sep  1 but at Sep  2 it does.

This sounds like an off-by-one in the day calculation, might be easy to fix. It's unclear though if this is a regression or has been like that always.
Comment 5 bjoern 2011-08-29 05:44:39 PDT
you might see a slightly different bug actually, can you please file a new bug for the off-by-one bug that you see? The initial report and the screen shot from https://bugzilla.mozilla.org/show_bug.cgi?id=371522#c23 show that the CRLs are not auto-updated at all.
Comment 6 Christian Holler (:decoder) 2011-08-29 05:54:21 PDT
Hm, I also tried the same steps with the CRL mentioned in bug 371522 screenshot attachment and besides the remark in comment 4, I cannot confirm the behavior. I imported the CRL, closed the browser and set a date after the next update field date, and auto-update status now says "OK".

@ Bjoern: Can you reproduce this on Fx 6, and if so, could you post steps to reproduce so I can try and reproduce it?
Comment 7 bjoern 2011-08-29 06:11:37 PDT
(In reply to Christian Holler (:decoder) from comment #6)
> Hm, I also tried the same steps with the CRL mentioned in bug 371522
> screenshot attachment and besides the remark in comment 4, I cannot confirm
> the behavior.

in any case you see a different bug in the same area, so please report it separately to not mix that issue up with the issue that people see here. If you do, please say which bug number that is so we can track changes from there, too.


> @ Bjoern: Can you reproduce this on Fx 6, and if so, could you post steps to
> reproduce so I can try and reproduce it?

I've seen this in *all* Firefox releases (Linux 32bit build only) that I can remember. There is not much more I can tell you to help to reproduce this except for adding the CRL and provide the URL of it. Maybe other people seeing the CRLs-never-updated bug can tell which OS build they see that bug on, maybe it is related to the operations system in some way.
Comment 8 Christian Holler (:decoder) 2011-08-29 06:36:58 PDT
(In reply to Bjoern Jacke from comment #7)

> in any case you see a different bug in the same area, so please report it
> separately to not mix that issue up with the issue that people see here. If
> you do, please say which bug number that is so we can track changes from
> there, too.

Bug 682830.

>
> I've seen this in *all* Firefox releases (Linux 32bit build only) that I can
> remember. There is not much more I can tell you to help to reproduce this
> except for adding the CRL and provide the URL of it. Maybe other people
> seeing the CRLs-never-updated bug can tell which OS build they see that bug
> on, maybe it is related to the operations system in some way.

I tried this on Windows because it was the first VM that I had available, but I will try on Linux in a few and let you know about the results.
Comment 9 Christian Holler (:decoder) 2011-08-29 07:23:33 PDT
Just tried it on Linux with Fx6. Added the CRL with auto-update set to 1 day before next-update date (next-update date for the CACert CRL I have is Sept 5). Then I changed date to a date after the update date (Sept 7 in my case), started Firefox again and waited a bit (it does not seem to do the update at start up). After some minutes, the auto-update status field showed "OK".
Comment 10 bjoern 2011-08-29 07:36:13 PDT
and I can reproduce the not-updating on a Windows XP 32bit machine with a fresh Firefox 6 installation instantly. As the "next update time" is defined in the CRL itself it's a bit tricky to tell if it works or not because even on a manual update the real next-update-time is shown taken from the info inside the CRL. I simply used wireshark to sniff all the traffic to the CRL server. So after add the CRL with URL http://crl.cacert.org/revoke.crl and enabling the auto-update, I shut down the VM, sniff all the traffci to crl.cacert.org, set the clock of the Test VM 60 days in the future, start it up and start Firefox. Even after several minutes you see no traffic going to crl.cacert.org.

Maybe the update only work if you (re)start your Firefox on the "next-update" day and it fails if Firefox is not being restarted or is just being (re)started some days later?
Comment 11 Christian Holler (:decoder) 2011-08-29 07:41:17 PDT
(In reply to Bjoern Jacke from comment #10)
> and I can reproduce the not-updating on a Windows XP 32bit machine with a
> fresh Firefox 6 installation instantly. As the "next update time" is defined
> in the CRL itself it's a bit tricky to tell if it works or not because even
> on a manual update the real next-update-time is shown taken from the info
> inside the CRL. I simply used wireshark to sniff all the traffic to the CRL
> server. So after add the CRL with URL http://crl.cacert.org/revoke.crl and
> enabling the auto-update, I shut down the VM, sniff all the traffci to
> crl.cacert.org, set the clock of the Test VM 60 days in the future, start it
> up and start Firefox. Even after several minutes you see no traffic going to
> crl.cacert.org.


I will try to repeat your exact setup. Is the "auto-update status" field remaining empty for you or does it show OK anyways?


> Maybe the update only work if you (re)start your Firefox on the
> "next-update" day and it fails if Firefox is not being restarted or is just
> being (re)started some days later?

At least on Linux, I did not start at the exact date at all, only afterwards, and it worked. Will try on Windows again but I think the problem is elsewhere. I also used https not http for the CRL.
Comment 12 bjoern 2011-08-29 07:47:43 PDT
(In reply to Christian Holler (:decoder) from comment #11)
> I will try to repeat your exact setup. Is the "auto-update status" field
> remaining empty for you or does it show OK anyways?

it remains empty

> I also used https not http for the CRL.

that doesn't make a difference for me.
Comment 13 Christian Holler (:decoder) 2011-08-29 08:47:45 PDT
(In reply to Bjoern Jacke from comment #12)

> it remains empty

I just tried this on Windows XP (32 bit) with a fresh install of Firefox 6, using the exact same CRL as you did (with HTTP), then closed FF and set the time to be November 2011. 3 minutes after opening Firefox again, my status field went to "OK". I did not check with Wireshark but given that your status remains empty and mine updates, that isn't necessary I guess. How long did you wait for the update to happen?
Comment 14 bjoern 2011-08-29 09:01:47 PDT
(In reply to Christian Holler (:decoder) from comment #13)
> I did not check with Wireshark but given that your
> status remains empty and mine updates, that isn't necessary I guess. How
> long did you wait for the update to happen?

more than an hour with firefox running idle in the VM. wireshark showing *no* access to the CRL server at all, status field remaining empty.
Comment 15 mozthun 2011-09-03 07:48:00 PDT
I tried to use Auto-Update of CRL with several machines (WinXP, Win7) and different versions of Thunderbird and Firefox with the CRLs from CAcert;
it never worked (status field remaining empty)

Then I tried Knoppix5.1.1EN and it worked instantly, but Knoppix5.1.1DE doesn't.
Also Knoppix6.7.0EN works, while Knoppix6.7.0DE doesn't.

with EN, status field shows OK or an error
with DE, status field remains empty

Therefore this might be related to system or program language , so I would recommend to mention system and program language with the test results.

WINXP DE Firefox 6.0.1 DE not working
WIN7  DE Firefox 6.0.1 DE not working

KNOPPIX 5.1.1 EN working:
"Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.8) Gecko/20061105 Iceape/1.0.6 (Debian-1.0.6-1)"
"Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.9) Gecko/20061220 Thunderbird/1.5.0.9"
"Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20061205 Iceweasel/2.0.0.1 (Debian-2.0.0.1+dfsg-1)"

KNOPPIX 5.1.1 DE not working:
"Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.8) Gecko/20061105 Iceape/1.0.6 (Debian-1.0.6-1)"
"Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.8.0.9) Gecko/20061220 Thunderbird/1.5.0.9"
"Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.8.1.1) Gecko/20061205 Iceweasel/2.0.0.1 (Debian-2.0.0.1+dfsg-1)"

KNOPPIX 6.7.0 DE not working:
"Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.18) Gecko/20110626 Icedove/3.1.11"
"Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0 Iceweasel/5.0"

KNOPPIX 6.7.0 EN working:
"Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.18) Gecko/20110626 Icedove/3.1.11"
"Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0 Iceweasel/5.0"
Comment 16 Christian Holler (:decoder) 2011-09-03 07:51:48 PDT
@mozthun: Thank you for your detailed analysis! Indeed I was only trying to reproduce this issue with EN builds, not with DE builds. I will try this out either today or tomorrow and report back if I can confirm.
Comment 17 mozthun 2011-09-04 02:51:14 PDT
I finally traced it done to a single setting
and Auto-Update worked the first time on this WinXP DE machine.

The problem is the system date format;
it's working with MM/DD/YYYY, but not with DD.MM.YYYY
The value in security.crl.autoupdate.nextInstant... is saved with the date format of the system; a soon as you change nextInstant to the date format MM/DD/YYYY on a system with date format DD.MM.YYYY , CRL Auto-Update works.

To reproduce this bug, set you system date format to DD.MM.YYYY
(e.g. with German regional settings)
Comment 18 Christian Holler (:decoder) 2011-09-05 04:46:32 PDT
After setting system date format to DD.MM.YYYY (using german regional settings on Windows XP), I can confirm the behavior. With that locale, I get this value for nextInstant:

security.crl.autoupdate.nextInstant.http://www.CAcert.org;11.09.2011 08:52:56 

Even after setting my date to 15.09.2011 and waiting, nothing happens, no CRL update although auto-update is enabled.
Comment 19 Christian Holler (:decoder) 2011-09-05 08:32:12 PDT
I don't think this is a regression. The data in the nextInstant field should be stored as a time_t, storing a date string will never work (and probably never did with a locale that uses a different date format). I assume this feature never worked as intended on those locales.

It is still unclear though how this will be fixed, as I was told there is some major work in progress that will replace this subsystem (and possibly others related to certificate management).
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-06 08:10:36 PDT
*** Bug 645555 has been marked as a duplicate of this bug. ***
Comment 21 Kaspar Brand 2011-09-06 11:41:14 PDT
(In reply to Christian Holler (:decoder) from comment #19)
> I don't think this is a regression. The data in the nextInstant field should
> be stored as a time_t, storing a date string will never work

Fully agree. Hard to understand why the code in nsCRLManager.cpp goes into all the contortions of converting a PRTime (PRInt64) value into a localized time string...

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsCRLManager.cpp&rev=&cvsroot=/cvsroot&mark=493-496#488

(Converting it back won't work for quite a few locales, I assume - see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/misc/prtime.c&rev=3.44&mark=951-967#945)

> I assume this feature never worked as intended on those locales.

Very likely. The code has basically been left untouched for about 9 years.

> It is still unclear though how this will be fixed, as I was told there is
> some major work in progress that will replace this subsystem (and possibly
> others related to certificate management).

What subsystem exactly? The CRLManager in PSM?
Comment 22 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-09-06 11:48:24 PDT
(In reply to Kaspar Brand from comment #21)
> What subsystem exactly? The CRLManager in PSM?

I think he is refering to the switch to libpkix for cert chain validation. The change to libpkix will not affect this.
Comment 23 Christian Holler (:decoder) 2011-09-07 03:31:52 PDT
(In reply to Brian Smith (:bsmith) from comment #22)
> 
> I think he is refering to the switch to libpkix for cert chain validation.
> The change to libpkix will not affect this.

Yep that's what I meant. But kaie said this would also affect the update procedure for CRLs.

@kaie: Can you give us some feedback here? Thanks :)
Comment 24 Kaspar Brand 2011-09-08 09:36:43 PDT
Moving to PSM.
Comment 25 bjoern 2011-11-18 13:53:26 PST
why don't you just close this bug as WORKSFORME if you don't care that people from non-English time zones should get CRL updates. Certificate Revocation List updates are nothing of importance, are they?
Comment 26 Christian Holler (:decoder) 2011-11-18 13:58:20 PST
(In reply to Bjoern Jacke from comment #25)
> why don't you just close this bug as WORKSFORME if you don't care that
> people from non-English time zones should get CRL updates. Certificate
> Revocation List updates are nothing of importance, are they?

I don't see how your comment helps here. If you want to develop a patch yourself, go for it, post it here and you will get feedback on it. If you don't want to do that, then you will have to wait until someone else does so. Brian has already been assigned this bug, but I assume he is still very busy with other bugs that are considered even more critical right now.
Comment 27 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-12-23 23:59:08 PST
The problem is that we use nsIDateTimeFormat::FormatPRExplodedTime to format the time to update the CRL. FormatPRExplodedTime ""performs a locale sensitive date formatting operation on the PRExplodedTime." However, this should be a locale-*INSENSITIVE* formatting operation that always formats the time in "1-Jan-1970 00:00:00 GMT" format.

http://hg.mozilla.org/mozilla-central/annotate/9107c2de89d6/security/manager/ssl/src/nsCRLManager.cpp#l477
Comment 28 Josh Aas 2012-01-10 07:43:09 PST
(In reply to Brian Smith (:bsmith) from comment #27)

Thanks Brian. When I looked into your theory briefly I thought perhaps the formatting wasn't locale sensitive because we pass NULL for the locale, but that just selects the current locale. Would be nice if the API docs for FormatPRExplodedTime noted that.
Comment 29 Steve Workman [:sworkman] (please use needinfo) 2012-01-24 17:05:59 PST
Created attachment 591332 [details] [diff] [review]
v1.0 patch

Patch ready but I have a question re resetting the current value of the pref for affected users.

Current changes:
 - Change nsCRLManager::ComputeNextAutoUpdateTime to format time using PR_FormatTime with a fixed format - uses GMT everywhere
 - Remove includes, dependencies for nsIDateTimeFormat in nsCRLManager.cpp

Resetting pref:
 - Spoke with Brian earlier: he suggested resetting the value in the pref when it isn't parsed correctly.  It looks like this should happen in nsNSSComponent::getParamsForNextCrlToDownload() which is called from DefineNextTimer() called at init/startup.  Currently, if security.crl.autoupdate.nextInstant for a particular CRL cannot be parsed, it is just skipped ('continue').  I have change it so that if the value cannot be parsed, a time is setup for 30 secs from now (PR_Now() + CRL_AUTOUPDATE_DEFAULT_DELAY, set in DefineNextTimer()).

Let me know if this is acceptable.  It seems like the CRL(s) should be updated and the pref reset.  There is a chance that the pref may be unparseable for another reason e.g. corruption, in which case timers will be firing every 30 secs.  If this isn't acceptable, any suggestions?
Comment 30 Kaspar Brand 2012-01-25 00:58:18 PST
I fail to understand why you're keeping a mechanism which is based on PR_FormatTime()/PR_ParseTimeString()... as has been suggested in comment 19, it would be much more reliable to simply use a time_t value (or PRTime, to be precise) for the nextInstant pref. Converting forth and back just makes it more error-prone.
Comment 31 Steve Workman [:sworkman] (please use needinfo) 2012-02-02 10:34:40 PST
Hi Kaspar, I was looking more at comment 27 and comment 28, which were discussing a quick fix in terms of locale.  After discussion with Brian, we decided to remove locale altogether and just use PRFormat (to reduce some dependencies etc.). But that's just a description of a train of thought that started with the assumption that using a formatted date was the way to go, not necessarily the correct train out of the station.  I'm happy either way - an advantage of using a formatted date could be easier debugging?  But, like I said, I'm happy either way on this.
Comment 32 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-15 14:11:03 PST
Steve, please re-post the patch with 8 lines of context and with the hg function names option turned on:

[defaults]
diff=-U 8 -p
qdiff=-U 8 -p
qnew = -U

[diff]
git = true
showfunc = true
unified = 8
Comment 33 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-15 14:11:40 PST
Comment on attachment 591332 [details] [diff] [review]
v1.0 patch

Clearing review while we wait for updated patch
Comment 34 Steve Workman [:sworkman] (please use needinfo) 2012-02-15 15:19:36 PST
Created attachment 597580 [details] [diff] [review]
v1.1 Fixed Diff format

Sorry for that.  I had those settings in my .hgrc but they didn't apply to 'export' apparently.
Comment 35 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-26 23:35:30 PST
Comment on attachment 597580 [details] [diff] [review]
v1.1 Fixed Diff format

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

Kai has suggested that we shouldn't fix this bug because we should remove this feature after we add support for auto-fetching CRLs. However, I do not know if/when we will actually support auto-fetching of CRLs, and the fix seems pretty simple, so IMO we should just fix this.

::: security/manager/ssl/src/nsCRLManager.cpp
@@ +472,4 @@
>      }
>    }
>  
> +  int autoUpdateStrLenMax = 25;

const int autoUpdateStrLenMax = 25;

Document how you determined that 25 is the correct maximum buffer size.

@@ +478,3 @@
>    PR_ExplodeTime(tempTime, PR_GMTParameters, &explodedTime);
> +  // Format as "1-Jan-1970 00:00:00 GMT"
> +  if (0 == PR_FormatTime(nextAutoUpdateDate, autoUpdateStrLenMax, "%v %T GMT",

PR_FormatTime is a deprecated function. It seems like a good idea to just format/parse the value as a number, as Kasper suggested, and then log the formatted version when logging is turned on (Perhaps just by printing the elements of the exploded time structure directly using PR_smprintf.
Comment 36 Steve Workman [:sworkman] (please use needinfo) 2012-03-03 17:35:06 PST
Created attachment 602682 [details] [diff] [review]
v2.0 removed PRFormatTime

Removed PRFormatTime; using PRTime type and storing as a string in prefs.  No Int64 or PRTime type in prefs, so storing as a string. Another option would be to store as two int32s; strings seem simpler in terms of keeping the diff small and less invasive.
Using nsCRT::atoll and PR_smprintf to convert between string and Int64. If there are better APIs, let me know and I'll update the diff.
Comment 37 Steve Workman [:sworkman] (please use needinfo) 2012-03-03 17:39:57 PST
Created attachment 602683 [details] [diff] [review]
v2.1 Minor build fixes

Same as last patch, had skipped 'hg qref' before uploading.
Comment 38 Steve Workman [:sworkman] (please use needinfo) 2012-03-05 13:17:39 PST
Try Server Results: https://tbpl.mozilla.org/?tree=Try&rev=7ee71edcc534

As of this comment, all is green except for Android Opt jsreftest-1, which I've respun. It's fine on Android XUL Opt, and I don't expect it to be different for native.
Comment 39 Kai Engert (:kaie) 2012-03-20 10:12:14 PDT
I would have prefered some comments in the code that explain why you assume your code works fine.

After reading the code, if I understand correctly, anyone already having old CRL-update-preferences, and using this new code for the first time, would experience the following:

- old pref is read, reading succeeds
- code will attempt to use atoll to convert the pref string to PRTime

Various contents for the old pref string are possible, for the various locales.

It could either start with a "day of month number" or with a "month number" or with a "year number". It seems that won't be a problem, because the 64bit number of PRTime seems to never wrap until year 586912, if I calculated correctly? If that's true, ignoring that distant future, we can assume that PRNow will always be >= than these very small numbers that will be seen when the old pref strings starts with a numerical day, month or year.

However, what about special scenario "zero"?

Could the old date string ever start with a "day of week name" or with a "month name", which would result in a zero return value from atoll? If that's true, then all iterations through all the various CRLs would give the zero value.

In this scenario, variable nearestUpdateTime would still have value zero after the loop. And in that scenario function getParamsForNextCrlToDownload returns a failure.


What does this mean?

If you can prove that the old pref string will never start with a character, then your patch seems OK to me.

If you cannot rule that out, or if we want to prepare for the scenario that the result of atoll might be zero for whatever reason, then I propose you test your patch with that scenario and make sure it will still work. You might have to adjust that "if(nearestUpdateTime > 0)" check.

Or, if my thinking is wrong, please explain why.
Comment 40 Steve Workman [:sworkman] (please use needinfo) 2012-03-20 16:49:06 PDT
Kai, thanks for the review comments. I've come up with a list of possible return values that I want to run by you.

So, nsCRT::atoll parses the first token in the string; I see 4 cases based on the type of token parsed:
 -1- Date number (year, month or day). Sample return values: 
     2012, 2013, 1-12, 1-31 -> All values far smaller than PR_Now()
 -2- Alpha char: returns 0; change to PR_Now() and force update.
 -3- Corrupted value (between epoch and PR_Now()): e.g. 0 - 1332280017
     (Tue Mar 20, 2012, 2:46pm approx): autoupdate will happen
 -4- Corrupted value (larger than PR_Now()): no autoupdate

Scenarios 1 and 2 seem most likely for users of CRL autoupdate, resulting in a forced autoupdate and restting the pref to use PRTime as an integer. Note that 2 would be a added condition to my previous patch, something like:

  if (tempTime == 0)
    tempTime = PR_Now();

.. just after the pref's value string is parsed.

3 would also trigger an autoupdate (timer set for now + CRL_AUTOUPDATE_DEFAULT_DELAY=30s, right?), and the pref will be given a PRTime value for next update; an unflagged recovery from corruption.
Update for 4 will be scheduled according to the value returned (assuming I'm reading DefineNextTimer() correctly); an unflagged corruption with no recovery.

I don't know how likely corruption would be, or how much you care about it, but I wanted to mention the possibilities here, specifically the fact that there would be no flagging of the error or the recovery in case 3. Seems to me that this is a corner case we can ignore, but I want to check with you before continuing.
Comment 41 Steve Workman [:sworkman] (please use needinfo) 2012-03-21 12:41:21 PDT
Created attachment 608059 [details] [diff] [review]
v2.2 Deal with time string to integer = 0

Updated patch adds error handling for pref string with a time format starting with alpha characters. Also adds comment based on scenarios identified in comment 40.

Tested by manually changing the pref's value to non-numeric characters. CRL is updated and pref reset to PRTime-based value.
Comment 42 Kai Engert (:kaie) 2012-03-22 06:36:21 PDT
Comment on attachment 608059 [details] [diff] [review]
v2.2 Deal with time string to integer = 0

Thank you for the changes, looks good to me, r=kaie
Comment 43 Kai Engert (:kaie) 2012-03-22 06:37:48 PDT
I think we don't need to worry about scenario 4.
Comment 44 Steve Workman [:sworkman] (please use needinfo) 2012-03-22 10:14:29 PDT
Thanks Kai.

FYI...
Try push: http://hg.mozilla.org/try/pushloghtml?changeset=02ad8e67b8a0
Results page: https://tbpl.mozilla.org/?tree=Try&rev=02ad8e67b8a0
Repo updated before push.
Comment 45 Steve Workman [:sworkman] (please use needinfo) 2012-03-23 10:35:40 PDT
Update on try: Please ignore the job in comment 44; it pushed an unrelated revision, not written by me.  Updated job results are here: https://tbpl.mozilla.org/?tree=Try&rev=261986253f0b
All seems to pass except Android Reftest J1, which I've respun a couple of times with no success. I doubt it's related to this patch, and I notice other people are seeing it as well.
Comment 46 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-23 14:09:27 PDT
Comment on attachment 608059 [details] [diff] [review]
v2.2 Deal with time string to integer = 0

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

I see no problems with this approach. I appreciate the logging to make debugging easier.
Comment 47 Steve Workman [:sworkman] (please use needinfo) 2012-03-23 15:27:14 PDT
Thanks Brian and Kai.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5904fd174eec
Comment 48 Ed Morley [:emorley] 2012-03-24 13:42:22 PDT
Please can you set the target milestone when landing on inbound, along the lines of http://blog.bonardo.net/2012/03/23/how-you-can-help-mozilla-inbound-sheriffs-when-pushing :-)

https://hg.mozilla.org/mozilla-central/rev/5904fd174eec
Comment 49 Steve Workman [:sworkman] (please use needinfo) 2012-03-26 10:46:43 PDT
Sorry Ed! Missed that one on my final todo list. Thanks!

Note You need to log in before you can comment on or make changes to this bug.