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)
Firefox
Address Bar
Tracking
()
VERIFIED
FIXED
Firefox 4.0b1
People
(Reporter: danne.da, Assigned: mak)
References
Details
(Keywords: privacy, regression)
Attachments
(3 files, 1 obsolete file)
15.93 KB,
application/zip
|
Details | |
39.59 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
2.43 KB,
patch
|
Details | Diff | Splinter Review |
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.
Regression window changeset: Works: fe937d72a9ce (April 23 nightly) Breaks: d542ee95ef22 (April 24 nightly)
Assignee | ||
Comment 2•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fe937d72a9ce&tochange=d542ee95ef22 i don't see any History related push in this range...
Comment 3•14 years ago
|
||
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
Updated•14 years ago
|
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.
Assignee | ||
Comment 6•14 years ago
|
||
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.
Assignee | ||
Comment 8•14 years ago
|
||
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.
Reporter | ||
Comment 10•14 years ago
|
||
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
Assignee | ||
Comment 11•14 years ago
|
||
yeah this makes more sense, thanks.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → mak77
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Updated•14 years ago
|
Keywords: regression
Reporter | ||
Comment 12•14 years ago
|
||
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.
Assignee | ||
Comment 13•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
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.
Assignee | ||
Comment 15•14 years ago
|
||
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
Assignee | ||
Comment 16•14 years ago
|
||
- 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)
Assignee | ||
Updated•14 years ago
|
Flags: in-testsuite?
Updated•14 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 17•14 years ago
|
||
(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 18•14 years ago
|
||
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+
Assignee | ||
Comment 19•14 years ago
|
||
(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.
Assignee | ||
Updated•14 years ago
|
Summary: Awesome bar history isn't cleared on exit → Ensure correct Places shutdown sequence and avoid sync expiration stuff.
Assignee | ||
Comment 20•14 years ago
|
||
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
Assignee | ||
Comment 21•14 years ago
|
||
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.
Reporter | ||
Comment 22•14 years ago
|
||
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.
Assignee | ||
Comment 23•14 years ago
|
||
thank you very much for reporting back! Glad this fixed the problem!
Assignee | ||
Comment 24•14 years ago
|
||
marking verified based on comment 22 and unit test passing.
Status: RESOLVED → VERIFIED
Assignee | ||
Updated•14 years ago
|
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)
Assignee | ||
Updated•14 years ago
|
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Whiteboard: [c-n attachment 451922 on m-c]
Assignee | ||
Comment 25•14 years ago
|
||
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.
Description
•