UMR in nsNavHistoryExpire::OnQuit()

RESOLVED FIXED

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Robert Sayre, Assigned: sdwilsh)

Tracking

({fixed1.9.1})

unspecified
x86
Mac OS X
fixed1.9.1
Points:
---
Bug Flags:
blocking-firefox3.5 +
wanted1.9.0.x ?
in-testsuite -
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
==47223== Conditional jump or move depends on uninitialised value(s)
==47223==    at 0x3B477DB: nsNavHistoryExpire::OnQuit() (nsNavHistoryExpire.cpp:204)
==47223==    by 0x3B325B5: nsNavHistory::Observe(nsISupports*, char const*, unsigned short const*) (nsNavHistory.cpp:5569)
==47223==    by 0x3CB43FE: nsObserverList::NotifyObservers(nsISupports*, char const*, unsigned short const*) (nsObserverList.cpp:128)
==47223==    by 0x3CB53E7: nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*) (nsObserverService.cpp:181)
==47223==    by 0x396CC4F: nsAppStartup::Quit(unsigned int) (nsAppStartup.cpp:254)
...
==47223==  Uninitialised value was created by a stack allocation
==47223==    at 0x3B47694: nsNavHistoryExpire::OnQuit() (nsNavHistoryExpire.cpp:181)
(Assignee)

Comment 1

9 years ago
This trunk or branch?  If it's branch, this should probably block (and I can own).
Flags: blocking-firefox3.5?
This bug is most certainly present on branch.  OnQuit has:

201   PRBool sanitizeOnShutdown, sanitizeHistory;
202   prefs->GetBoolPref(PREF_SANITIZE_ON_SHUTDOWN, &sanitizeOnShutdown);
203   prefs->GetBoolPref(PREF_SANITIZE_ITEM_HISTORY, &sanitizeHistory);
204   if (sanitizeHistory && sanitizeOnShutdown)
205     return;

line 92 -- #define PREF_SANITIZE_ON_SHUTDOWN  "privacy.sanitize.sanitizeOnShutdown"

This pref is only set in firefox.js.  Other apps that use toolkit get a random value here.

line 93 -- #define PREF_SANITIZE_ITEM_HISTORY  "privacy.item.history"

This pref is never set anywhere; this value is just random.

Both GetBoolPref() calls should probably check rv and do something sane if the pref is not set.  Or something.  And maybe there should be default values in all.js.
Assignee: nobody → sdwilsh
So that looks like it'll lead to people getting sanitizing on shutdown who don't want it?  Eek.  Patch ASAP ASAP pls!
Flags: blocking-firefox3.5? → blocking-firefox3.5+
No, vice versa, if I read the code right.  It'll lead to not deleting data we should (e.g. old history?) because we think that we'll sanitize when in fact we will not.

That said, the default value of "privacy.sanitize.sanitizeOnShutdown" is false, so this bug can only trigger if "privacy.sanitize.sanitizeOnShutdown" is set to true but "privacy.item.history" remains unset.  Which I suspect is quite possible!
(Assignee)

Comment 5

9 years ago
I wonder if this is the reason why shutdown takes so long for some people...

I can cook up a patch tonight if this is really ASAP, or I can do it first thing in the morning when I get in (between 8 and 9 PDT).
Status: NEW → ASSIGNED
ASAP, please -- we want to spin tonight if we can get the other patches moved through the system and the video-sync bug sorted.
(Assignee)

Comment 7

9 years ago
OK - pulling and building (but build will take a while - this is a first gen macbook).
(Reporter)

Comment 8

9 years ago
This is from branch.
(Assignee)

Comment 9

9 years ago
Created attachment 380990 [details] [diff] [review]
v1.0

This would only result in us doing more work on shutdown.  I've done what we end up doing elsewhere in places when dealing with preferences and rely on XPCOM rules (outparams are not touched if a failure occurs) and set a sane default.

Attached patch already has a commit message (and assumes dietrich will grant r+ on this) on the chance that I've managed to not stay awake (feel like a zombie as it is).
Attachment #380990 - Flags: review?
(Assignee)

Comment 10

9 years ago
Regressed from bug 402297.  That also means we probably want to fix this on branch.
Blocks: 402297
Flags: wanted1.9.0.x?
Comment on attachment 380990 [details] [diff] [review]
v1.0

r=me, thanks for quick turnaround
Attachment #380990 - Flags: review? → review+
requesting in-litmus, since we need to test with a manual shutdown, and restart.
Flags: in-testsuite-
Flags: in-litmus?
(Assignee)

Comment 13

9 years ago
http://hg.mozilla.org/mozilla-central/rev/eac1cf3d71f1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7376f75b8848
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED

Updated

9 years ago
Duplicate of this bug: 455058

Comment 15

9 years ago
> I wonder if this is the reason why shutdown takes so long for some people...

FWIW, it didn't help for me.  Still takes over a minute in the same cases.
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.