1029128 - Fix a couple of small things around the password manager storage code

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Trying to get the patch for bug 949617 landed, I ended up having to do some spelunking in the password manager store. While there, I found one strict mode violation that, as far as I know, is not causing any problems and one bug that was biting me when I was trying to debug some mochitests alone (it wouldn't bite on try and I doubt it would cause real dataloss in the wild). More explanation on the patch itself.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix bug in failure case biting debugging mochitest failures.

In order for this bug to bite, we have to modify the data store, when there is no logins.json file to read before we're notified of the error to read the file. The fix is pretty straightforward.

Whoever wants to review this can take it :-)

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix strict-mode error in the JSON storage file.

No commentary needed here, really. Easy fix. I'll make sure to send this through try since these patches tend to turn things orange.

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 8444692 [details] [diff] [review]
Fix bug in failure case biting debugging mochitest failures.

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

Oh, hmm. So if the synchronous-load path [via ensureDataReady()] gets triggered by some operation, things are OK when OS.File.read() works normally because we immediately check .dataReady in the try{}... But if it throws, we never check .dataReady, and (re)startup with empty storage. Most times this is probably ok, since the sync path should also end up with empty storage. But if the thing triggering that path _modified_ storage in the process, the async restartup would clobber it. Oops.

Ideally this new check should be at the top of the catch{}, just makes it clear that if the sync path executed we don't need to try and backup a corrupt file. Oh, but haha, the exact same bug would then happen if the sync path got triggered while we were asynchronously backing up the .corrupt file. :| I suppose that could just be spun off as a new Task, since we don't really need to wait for it to finish, but meh.

Comment 5

[:Paolo Amadini]

Thanks Blake for fixing this possible race condition!

I suggest adding a comment to the patch to explain what could go wrong there (brief summary of comment 4).

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f568b1dc9880
https://hg.mozilla.org/integration/mozilla-inbound/rev/033d6271fd19

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Something in this push made Android sadface. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f291b1b47c7

https://tbpl.mozilla.org/php/getParsedLog.php?id=42378026&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=42378331&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=42377296&tree=Mozilla-Inbound

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Take 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/958aaea55e3f

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Also: https://hg.mozilla.org/integration/mozilla-inbound/rev/df7bcbbeda21

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/958aaea55e3f
https://hg.mozilla.org/mozilla-central/rev/df7bcbbeda21

Comment 11

[Carsten Book [:Tomcat]]

sorry had to back this out from mozilla-central (and so with the merge in the integration trees) in https://tbpl.mozilla.org/?rev=83f9b25520bf since one of this checkins seems to have caused Bug 1030663 which is a frequent test failure now

Comment 12

[:Paolo Amadini]

(In reply to Carsten Book [:Tomcat] from comment #11)
> sorry had to back this out from mozilla-central (and so with the merge in
> the integration trees) in https://tbpl.mozilla.org/?rev=83f9b25520bf since
> one of this checkins seems to have caused Bug 1030663 which is a frequent
> test failure now

This changeset is the least likely to have caused the failure. If the culprit is this one, we're missing something important.

What is required to verify and re-land this changeset? Can you help with that process?

Comment 13

[Carsten Book [:Tomcat]]

hey paolo, i guess its ok to reland if you guys are sure its not that one. (i guess otherwise i would guess a try run is enough confirmation this is not the one that caused the other bug :)

Comment 14

[:Paolo Amadini]

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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b294b77e8095
https://hg.mozilla.org/mozilla-central/rev/c124393b6eac