urlclassifier opens db connection even if it's preffed off

RESOLVED INVALID

Status

()

Toolkit
Safe Browsing
RESOLVED INVALID
7 years ago
4 years ago

People

(Reporter: khuey, Unassigned)

Tracking

({footprint})

unspecified
Firefox 7
footprint
Points:
---

Firefox Tracking Flags

(firefox6 unaffected, firefox7 unaffected, firefox8 unaffected)

Details

Attachments

(2 attachments)

Created attachment 543321 [details] [diff] [review]
Patch

I have urlclassifier preffed off because I don't want anything to do with it :-)

I noticed in about:memory that urlclassifier3.sqlite is still loaded and burning some memory.  This patch changes the code to not open a db connection if it's preffed off.  I've been running with this patch all day and haven't run into any problems yet.
Attachment #543321 - Flags: review?(dcamp)
Does this handle the case of the user turning the pref back on? I suppose we could make toggling the db require a restart but then we should probably change the pref dialog to reflect that.

Comment 2

7 years ago
Comment on attachment 543321 [details] [diff] [review]
Patch

NS_ENSURE_SUCCESS spews on failure right?  Probably better to just remove that NS_ERROR and keep the if(failed) { return; }
Attachment #543321 - Flags: review?(dcamp) → review+
(In reply to comment #1)
> Does this handle the case of the user turning the pref back on? I suppose we
> could make toggling the db require a restart but then we should probably
> change the pref dialog to reflect that.

Yes, I turned it back on and went to the its-a-trap page and it caught it.  Turning it back off doesn't unload the db of course, but I can live with that.

(In reply to comment #2)
> Comment on attachment 543321 [details] [diff] [review] [review]
> Patch
> 
> NS_ENSURE_SUCCESS spews on failure right?  Probably better to just remove
> that NS_ERROR and keep the if(failed) { return; }

Yes.  Thanks for the quick review.
http://hg.mozilla.org/mozilla-central/rev/549e3276ed25
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7

Updated

7 years ago
Keywords: footprint
Apparently we can't use prefs off the main thread, so this needs to come out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 544084 [details] [diff] [review]
Backout

We'll need to back this out of Aurora too.
Attachment #544084 - Flags: approval-mozilla-aurora?
Attachment #544084 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/9cd64dc8d584
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → INCOMPLETE
why resolved, it still seems like a valid fix even if the patch was wrong. Is there no other way to read the pref on main thread and pass its value to the worker?
(In reply to comment #9)
> why resolved, it still seems like a valid fix even if the patch was wrong.
> Is there no other way to read the pref on main thread and pass its value to
> the worker?

I've resolved it incomplete because I didn't finish here, and don't have any intentions of doing so at the moment.

It's tricky because OpenDB is called on the background thread, so you'd have to block while getting the pref value on the main thread.  It's probably easier to figure out what's causing OpenDB to be called and conditioning that on the pref.
that is still what this bug is about, I don't think we need one bug per approach.
reopening and unassigning, the bug is valid, it may be harder to fix than expected but that's fine.
Assignee: khuey → nobody
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---

Updated

7 years ago
status-firefox6: --- → unaffected
status-firefox7: --- → unaffected
status-firefox8: --- → unaffected
Calling this invalid now, because the sqlite DB is no longer used.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago5 years ago
Resolution: --- → INVALID
(Assignee)

Updated

4 years ago
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.