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

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

unspecified
mozilla33
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Assignee: nobody → mrbkap
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 :-)
Attachment #8444692 - Flags: review?(paolo.mozmail)
Attachment #8444692 - Flags: review?(dolske)
No commentary needed here, really. Easy fix. I'll make sure to send this through try since these patches tend to turn things orange.
Attachment #8444693 - Flags: review?(dolske)
Attachment #8444693 - Flags: review?(dolske) → review+
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.
Attachment #8444692 - Flags: review?(paolo.mozmail)
Attachment #8444692 - Flags: review?(dolske)
Attachment #8444692 - Flags: review+
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).
https://hg.mozilla.org/mozilla-central/rev/958aaea55e3f
https://hg.mozilla.org/mozilla-central/rev/df7bcbbeda21
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
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 :)
https://hg.mozilla.org/mozilla-central/rev/b294b77e8095
https://hg.mozilla.org/mozilla-central/rev/c124393b6eac
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.