Closed Bug 578557 Opened 10 years ago Closed 10 years ago

Better handling of expiration with clean history

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b2

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached file patch v1.0 (obsolete) —
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch patch v1.0 (obsolete) — Splinter Review
Attachment #457202 - Attachment is obsolete: true
Attached patch patch v1.1 (obsolete) — Splinter Review
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.
Attachment #457203 - Attachment is obsolete: true
Attachment #457321 - Flags: review?(dietrich)
Blocks: 576590
Attached patch patch v1.2Splinter Review
but cleanup on idle only a dirty history.
Attachment #457321 - Attachment is obsolete: true
Attachment #457321 - Flags: review?(dietrich)
Attachment #457564 - Flags: review?(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 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 on attachment 457564 [details] [diff] [review]
patch v1.2

talked on irc, r=me
Attachment #457564 - Flags: review?(dietrich) → review+
http://hg.mozilla.org/mozilla-central/rev/59539a9891b0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b2
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
You need to log in before you can comment on or make changes to this bug.