Closed Bug 1007850 Opened 6 years ago Closed 6 years ago

Don't do any work in Seer::Reset if disabled

Categories

(Core :: Networking, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox30 + verified
firefox31 + verified
firefox32 + verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: u408661, Assigned: u408661)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
Running quickly through try now: https://tbpl.mozilla.org/?tree=Try&rev=a8c7b9957f67
Attachment #8419618 - Flags: review?(mcmanus)
Attachment #8419618 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
Tracking this so we can evaluate on channels and see if this addresses some of the shutdown issues that are still presenting from bug 1005958
Welp, that failed on try, thanks to the lack of a try block in ForgetAboutSite.jsm around the call to reset the seer (which now throws). New patch incoming.
Keywords: checkin-needed
Attached patch patch v2Splinter Review
So this just makes the seer return NS_OK if it's disabled when Reset is called. The other option is to wrap the call in ForgetAboutSite.jsm in a try block to silence the error. I (slightly) prefer this version, however.

New try run to make sure I really, for reals, fixed it this time: https://tbpl.mozilla.org/?tree=Try&rev=cce26723eaa8
Attachment #8419618 - Attachment is obsolete: true
Attachment #8419732 - Flags: review?(mcmanus)
Attachment #8419732 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/7dd80b7cee40
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment on attachment 8419732 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 881804
User impact if declined: Seer does some work on shutdown if disabled and sanitize is enabled, causing an empty sqlite file to magically appear on disk if it wasn't there before.
Testing completed (on m-c, etc.): on m-c, locally
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8419732 - Flags: approval-mozilla-beta?
Attachment #8419732 - Flags: approval-mozilla-aurora?
Attachment #8419732 - Flags: approval-mozilla-beta?
Attachment #8419732 - Flags: approval-mozilla-beta+
Attachment #8419732 - Flags: approval-mozilla-aurora?
Attachment #8419732 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Does this fix need manual QA verification ?
Flags: needinfo?(hurley)
Not sure that I'm the best arbiter of whether or not a particular fix needs manual QA verification, but it seems like a waste of time for QA to spend any time verifying this.

That said, if you want to, here are steps to reproduce/verify:

(1) Set network.seer.enabled to false
(2) Set privacy.sanitize.sanitizeOnShutdown to true
(3) Quit firefox
(4) Remove <profile>/netpredictions.sqlite
(5) Start firefox (maybe browse around a bit, if you're having a boring day)
(6) Quit firefox
(7) See if <profile>/netpredictions.sqlite exists

In builds from before this fix landed, the file *will* exist in step 7. In builds from after this fix landed, the file should *not* exist in step 7.
Flags: needinfo?(hurley)
Verified on Windows 7 64bit using Beta 30.0b4, latest Nightly 32.0a1 and latest Aurora 31.0a2. Based on comment #10 I will mark this issue as verified. If you consider otherwise or want me to verify on other OSs please let me know.
You need to log in before you can comment on or make changes to this bug.