578557 - Better handling of expiration with clean history

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Expiration at shutdown is just an additional privacy protection for heavy changing profiles, it should not run usually, especially in short sessions. Similar stuff for idle, no reasons to observe it if history is clean enough.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.1

This one seems to be much better, with async visits patches we saw a 1000% regression in Ts Shutdown due to the fact it was not counting expiration (before) and was counting it (now), with this patch the regression is now at 30% that is the DBFlush effect.
This tries to avoid running idle and shutdown expiration steps unless they are really really needed.

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.2

but cleanup on idle only a dirty history.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 457564 [details] [diff] [review]
patch v1.2

>Bug 578557 - Better handling of expiration with clean history.
>
>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
>--- a/toolkit/components/places/src/nsNavHistory.cpp
>+++ b/toolkit/components/places/src/nsNavHistory.cpp

these navhistory changes don't look related to this patch. can you explain how they're related? if they're not, please file a different bug and move them there.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 457564 [details] [diff] [review]
patch v1.2

>Bug 578557 - Better handling of expiration with clean history.
>
>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
>--- a/toolkit/components/places/src/nsNavHistory.cpp
>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -5757,33 +5757,12 @@ nsNavHistory::Observe(nsISupports *aSubj
>     (void)os->RemoveObserver(this, TOPIC_AUTOCOMPLETE_FEEDBACK_INCOMING);
> #endif
> 
>-    // If shutdown happens in the same scope as the service init, we should
>-    // immediately serve the places-init topic, this way topic observers
>-    // won't try to access the database after xpcom-shutdown.
>-    nsCOMPtr<nsISimpleEnumerator> e;
>-    nsresult rv = os->EnumerateObservers(TOPIC_PLACES_INIT_COMPLETE,
>-                                         getter_AddRefs(e));
>-    if (NS_SUCCEEDED(rv) && e) {
>-      // This covers a special case that can happen in tests, if the test
>-      // does never interrupt the main thread we could shutdown
>-      // before we fire any notification.  That means that if we notify from now
>-      // on, we could init the category cache after xpcom-shutdown and leak.
>-      mCanNotify = false;
>-
>-      nsCOMPtr<nsIObserver> observer;
>-      PRBool loop = PR_TRUE;
>-      while(NS_SUCCEEDED(e->HasMoreElements(&loop)) && loop) {
>-        e->GetNext(getter_AddRefs(observer));
>-        (void)observer->Observe(observer, TOPIC_PLACES_INIT_COMPLETE, nsnull);
>-      }
>-    }
>-
>     // Notify all Places users that we are about to shutdown.  The notification
>     // is enqueued because there is network work on profile-before-change that
>     // should run before us.
>     nsRefPtr<PlacesEvent> shutdownEvent =
>       new PlacesEvent(TOPIC_PLACES_SHUTDOWN);
>-    rv = NS_DispatchToMainThread(shutdownEvent);
>+    nsresult rv = NS_DispatchToMainThread(shutdownEvent);
>     NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
>                      "Unable to shutdown Places: message dispatch failed.");
> 
>@@ -5797,6 +5776,11 @@ nsNavHistory::Observe(nsISupports *aSubj
>   }
> 
>   else if (strcmp(aTopic, TOPIC_PLACES_TEARDOWN) == 0) {
>+    // Don't even try to notify observers from this point on, the category
>+    // cache would init services that could not shutdown correctly or try to
>+    // use our APIs.
>+    mCanNotify = false;
>+
>     // Operations that are unlikely to create issues to implementers should go
>     // in global shutdown.  Any other thing that must run really late must be
>     // here instead.
>diff --git a/toolkit/components/places/src/nsPlacesExpiration.js b/toolkit/components/places/src/nsPlacesExpiration.js
>--- a/toolkit/components/places/src/nsPlacesExpiration.js
>+++ b/toolkit/components/places/src/nsPlacesExpiration.js
>@@ -254,7 +254,7 @@ const EXPIRATION_QUERIES = {
>        + "WHERE v.id IS NULL "
>        +   "AND v_t.id IS NULL "
>        +   "AND b.id IS NULL "
>-       +   "AND h.ROWID <> IFNULL(:null_skips_last, last_insert_rowid()) "
>+       +   "AND h.ROWID <> IFNULL(:null_skips_last, (SELECT MAX(ROWID) FROM moz_places_temp)) "
>        +   "AND SUBSTR(h.url, 1, 6) <> 'place:' "
>        + "UNION ALL "
>        + "SELECT h.id, h.url, h.last_visit_date AS visit_date, "
>@@ -266,7 +266,6 @@ const EXPIRATION_QUERIES = {
>        + "WHERE v.id IS NULL "
>        +   "AND v_t.id IS NULL "
>        +   "AND b.id IS NULL "
>-       +   "AND h.ROWID <> IFNULL(:null_skips_last, last_insert_rowid()) "
>        +   "AND SUBSTR(h.url, 1, 6) <> 'place:' "
>        + "LIMIT :limit_uris",
>     actions: ACTION.TIMED | ACTION.SHUTDOWN | ACTION.IDLE | ACTION.DEBUG

hrm, this looks unrelated too. is the idea that the only not-yet-fully-added visits we might accidentally expire would be in the temp table?

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 457564 [details] [diff] [review]
patch v1.2

talked on irc, r=me

Comment 7

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/59539a9891b0

Comment 8

[Marco Bonardo [:mak]]

pushed a irc-reviewed follow-up due to idle expiration running in the middle of tests on Mac.
http://hg.mozilla.org/mozilla-central/rev/2bfc4dd54872