Closed Bug 562644 Opened 14 years ago Closed 14 years ago

Ensure correct Places shutdown sequence and avoid sync expiration stuff. (Clear locationbar history on shutdown)

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 4.0b1

People

(Reporter: danne.da, Assigned: mak)

References

Details

(Keywords: privacy, regression)

Attachments

(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100429 Minefield/3.7a5pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100429 Minefield/3.7a5pre

The preferences is set to clear history when Firefox closes, but despite this the history remains the next time Firefox is started.

Reproducible: Always

Steps to Reproduce:
1. Mske sure Firefox is set to clear history on exit
2. Browse around to store some things in history
3. Close Firefox
4. Restart Firefox and start typing in the awesomebar.
Actual Results:  
Previous history is shown.

Expected Results:  
No previous history should be shown.

It does however seem to clear history if Firefox is only open for less than a minute or so, anything longer and the history remains.

Tested with all extensions and plugins disabled without any change.
Version: unspecified → Trunk
Regression window changeset:

Works: fe937d72a9ce (April 23 nightly)
Breaks: d542ee95ef22 (April 24 nightly)
You sure it is history, and not bookmarks showing up in the awesome bar?
Hmm, it appears as though that history _is_ cleared but the awesome bar history is not. The stuff that shows up are things from previous sessions.
Summary: History isn't cleared on exit → Awesome bar history isn't cleared on exit
Component: Bookmarks & History → Location Bar
QA Contact: bookmarks → location.bar
I looked inside of the places.sqlite file and found that after exiting Firefox the table moz_places still contained any history from the previous session. moz_historyvisits was empty.

If I cleared the cache manually moz_places was cleared of any history. 

moz_inputhistory however wasn't cleaned by either a manual wipe or wipe on exit.
moz_places does not contain history, it contains urls, and those are not necessarily history items. inputhistory is cleared when things are removed from moz_places (That are removed when visits are).
Now, are you 100% sure that what you see in the awesomebar are visited pages and not:
- bookmarks
- live bookmarks (RSS) entries

Can you reproduce the same problem in a new test profile?
I've attached a places.sqlite from a clean profile, where one can see history items remain in moz_places (ID 61 to 88), with moz_historyvisits and moz_inputhistory being empty.
The regression range does no make sense, but i'll try to reproduce and see what i can find. the expected behavior is that onClearHistory we should remove any dangling entry that has no visits nor bookmarks.
I need to revise the regression range to a day earlier. When I downloaded a bunch of builds earlier I must have gotten 4/22 and 4/23 nightly builds mixed up somehow. Now the regression range makes much more sense:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cdc8bf25220e&tochange=fe937d72a9ce

Bug 529821 messes with places so I suppose it's a good place to start looking.
Correct changeset, checked wrong folder on the ftp:

I did run 4/22 build, so the changeset should be: ab4c6ba5ff70

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ab4c6ba5ff70&tochange=fe937d72a9ce
yeah this makes more sense, thanks.
Assignee: nobody → mak77
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: regression
Blocks: 560198
Keywords: privacy
I was using Minefield on Windows today and I've got the bug there too. History is empty but visited urls remain in the awesomebar.
I was unable to reproduce the bug (by finding entries in the locationbar), but I've noticed a couple assertions when expiration tries to bind statements, and that is probably the same bug.  What seems to happen is that expiration receives places-shutdown before browserglue gets it, and expiration statements are already finalized by the time we are asked to cleanup by the clear history on shutdown call.
Maybe I also got an idea to speed up expiration, so I'll try it.
Attached patch patch v1.0 (obsolete) — Splinter Review
Something like this. I'm getting a strange assertion from session cache clear (called by this._sanitizer.onShutdown() in browserglue), but I can't see how this could have caused that.
Strange, it's asserting in NSS_RegisterShutdown
    if (!NSS_IsInitialized()) {
	PORT_SetError(SEC_ERROR_NOT_INITIALIZED);
	return SECFailure;
    }

nss is not initialized but we try to shutdown it?
This only happens if Clear "Active Logins" is checked
Attached patch patch v1.2Splinter Review
- make final expiration run enqueued (after clear history on shutdown)

- make expiration use a temp table for notifications. this avoids us to requery twice for the same entries and to collect results twice. Should be pretty faster.

- make single core limit near double core limit, I was too pessimistic when I set those. As soon as beta comes out we should make a new places stats call to check how results distribution has changed (I already filed a bug in the past about this).

- better solution for the "expiration could expire just added page".  Won't expire anymore the last added entry if it's called automatically (on idle or on timer). This removed the only sync statement that was in expiration code (yay!)

- use createAsyncStatement, less work.

- shutdown is now splitted in 2 parts. "places-shutdown" is the common notification everybody should listen at, things are still guaranteed to somehow work. "places-teardown" is for internal use, statements are finalized, last sync is run and db is closed, this is the "real" shutdown.

So profile-before-change makes Places enqueue a places-shutdown notification (I've discovered that NSS is unhappy if we try to clear security keys at profile-before-change, since some security work runs there) to all Places users and a places-teardown notification. Browserglue clears history and expiration enqueues a run for it.
When teardown is received, lazy messages are handled, statements are finalized, last sync is enqueued (so it goes after expiration) and database is async closed.
Sounds complicated but till we remove temp table and lazy stuff our shutdown IS complicated.
Attachment #450225 - Attachment is obsolete: true
Attachment #450523 - Flags: review?(dietrich)
Flags: in-testsuite?
Blocks: 563538
OS: Mac OS X → All
Hardware: x86 → All
(In reply to comment #16)
> When teardown is received, lazy messages are handled, statements are finalized,
> last sync is enqueued (so it goes after expiration) and database is async
> closed.

will the processing of lazy messages cause notifications? if so, could that cause problems for consumers of those notifications, who expect the places infrastructure to still be usable at that point?
Comment on attachment 450523 [details] [diff] [review]
patch v1.2

>+        if (aAction != ACTION.TIMED && aAction != ACTION.IDLE) {
>+          // NULL is automaticall bound otherwise.
>+          params.null_skips_last = -1;

typo

looks ok otherwise. outside of my question about lazy message processing at shutdown, r=me.
Attachment #450523 - Flags: review?(dietrich) → review+
(In reply to comment #17)
> (In reply to comment #16)
> > When teardown is received, lazy messages are handled, statements are finalized,
> > last sync is enqueued (so it goes after expiration) and database is async
> > closed.
> 
> will the processing of lazy messages cause notifications? if so, could that
> cause problems for consumers of those notifications, who expect the places
> infrastructure to still be usable at that point?

It is possible that it causes notifications, but firing them earlier means not being sure that all messages have been handled, and is not different from before (also before we were serving lazy messages and then finalizing stmts). That said when they fire the system is still working unless someone enqueues a request, but implementers should stop doing their stuff (and remove observers) at places-shutdown.
Summary: Awesome bar history isn't cleared on exit → Ensure correct Places shutdown sequence and avoid sync expiration stuff.
with fixed comment
http://hg.mozilla.org/mozilla-central/rev/3a8ffb0da077
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a6
looks like this test was acting wrongly and the patch fixed code, changing so that if a limit is not specified when forcing an expiration, no limit is used.
Will push when tree reopens.
I can confirm that I can no longer see this issue with the latest hourly build. The awesomebar does no longer contain any history-urls after shutdown.
thank you very much for reporting back! Glad this fixed the problem!
marking verified based on comment 22 and unit test passing.
Status: RESOLVED → VERIFIED
Summary: Ensure correct Places shutdown sequence and avoid sync expiration stuff. → Ensure correct Places shutdown sequence and avoid sync expiration stuff. (Clear locationbar history on shutdown)
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Whiteboard: [c-n attachment 451922 on m-c]
re-enable and fix test
http://hg.mozilla.org/mozilla-central/rev/17c92c9a6994
Keywords: checkin-needed
Whiteboard: [c-n attachment 451922 on m-c]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: