Last Comment Bug 725597 - SafeBrowsing fails to update persistent PrefixSet on Windows
: SafeBrowsing fails to update persistent PrefixSet on Windows
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Safe Browsing (show other bugs)
: 13 Branch
: x86_64 Windows Vista
: -- critical (vote)
: Firefox 17
Assigned To: Gian-Carlo Pascutto [:gcp]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-09 02:01 PST by Gian-Carlo Pascutto [:gcp]
Modified: 2014-05-27 12:25 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch. Fix WriteFile on Windows (3.06 KB, patch)
2012-02-09 11:18 PST, Gian-Carlo Pascutto [:gcp]
dcamp: review+
Details | Diff | Review
Patch. Backout (3.01 KB, patch)
2012-04-19 09:19 PDT, Gian-Carlo Pascutto [:gcp]
akeybl: approval‑mozilla‑aurora+
mark.finkle: approval‑mozilla‑central+
Details | Diff | Review

Description Gian-Carlo Pascutto [:gcp] 2012-02-09 02:01:19 PST
What seems to be happening is that upon the first update, all files are created normally. Upon the second update, saving the sbtore files succeeds but saving the pset files doesn't. The data in memory *is* updated, though.

When the user restarts the browser, our corruption detection will see that the sbstore data is not in sync with the pset data, and blow away the database.

Bugs are:
a) prefixSet->WriteFile(); in Classifier.cpp doesn't have its return value checked
b) Windows doesn't allow us to rename into the file (SafeOutputStream) while we have an open read handle to it. This is a known problem that was fixed in HashStore.cpp but missed in LookupCache.cpp
c) I think its possible to make a testcase for this, so we should probably do that.
Comment 1 Gian-Carlo Pascutto [:gcp] 2012-02-09 11:18:05 PST
Created attachment 595825 [details] [diff] [review]
Patch. Fix WriteFile on Windows

https://tbpl.mozilla.org/?tree=Try&rev=0ad1f7d95261

Adding a test for this specific failure seems tricky. I'd need to shut down the entire classifier system, afaik. Not sure how to achieve that.
Comment 2 Gian-Carlo Pascutto [:gcp] 2012-02-09 11:42:46 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9291f227d63
Comment 3 Ed Morley [:emorley] 2012-02-10 05:01:55 PST
https://hg.mozilla.org/mozilla-central/rev/e9291f227d63
Comment 4 Gian-Carlo Pascutto [:gcp] 2012-04-19 09:19:53 PDT
Created attachment 616610 [details] [diff] [review]
Patch. Backout

[Approval Request Comment]
Backout due to bug 744993:

a01cf079ee0b Bug 730247
1a6d008acb4f Bug 729928
f8bf3795b851 Bug 729640
35bf0d62cc30 Bug 726002
a010dcf1a973 Bug 726002
e9291f227d63 Bug 725597
db52b4916cde Bug 673470
173f90d397a8 Bug 673470
Comment 5 Gian-Carlo Pascutto [:gcp] 2012-04-19 22:50:55 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/46e19522aa98
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-20 10:44:20 PDT
https://hg.mozilla.org/mozilla-central/rev/46e19522aa98
Comment 7 Alex Keybl [:akeybl] 2012-04-24 07:11:12 PDT
Comment on attachment 616610 [details] [diff] [review]
Patch. Backout

Sorry - thought this bug was reopened because the backout was backed out. My mistake. Approved for Aurora 13 (or Beta 13 if the merge occurs before we land).
Comment 8 Gian-Carlo Pascutto [:gcp] 2012-05-01 06:42:27 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/eab0fd65945b
Comment 9 Gian-Carlo Pascutto [:gcp] 2012-08-15 00:14:25 PDT
Relanding after fixes in bug 673470 to fix bug 744993.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e63466828f6b
Comment 10 Ed Morley [:emorley] 2012-08-15 09:47:42 PDT
https://hg.mozilla.org/mozilla-central/rev/e63466828f6b

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