Closed Bug 499828 Opened 12 years ago Closed 10 years ago

kill LAZY_ADD and use async storage instead.

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b3
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 2 obsolete files)

Would be cool if we could throw away LAZY_ADD and use async storage.
Blocks: 337537
Depends on: 539706
Depends on: 540765
Blocks: 552023
No longer depends on: 540765
Depends on: 553489
Blocks: 550778
Insofar as this causes bug 550778, this should probably block 1.9.3.
blocking2.0: --- → ?
Depends on: 556631
Depends on: 560118
Doesn't seem like this blocks by itself. Bug 550778 is the blocker, if this fixes it then awesome.
blocking2.0: ? → -
Depends on: 556400
Depends on: 566738
Actually, while we should absolutely make async frecency calculation before final, this bug can be fixed as soon as visits and titles are async. Indeed async visits and titles don't pass through lazy messages at all and icons are already async.
frecency does not pass through lazy messages and will probably hurt till we fix it, but is unrelated to this.
Also Bug 550778 should disappear regardless status of this bug once visits are added async.

So next steps (not necessarily in this order) are:
- fix this (bye bye laziness)
- make frecency async
- upgrade to sqlite 3.7.0
- use WAL journaling
- kill temp tables
- evaluate and kill any remaining sync step that could still exist in the full process.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch wip patch (obsolete) — Splinter Review
additional fixes, since Shawn is reviewing the first wip patch, this is like an interdiff, will merge patches after review.
Attachment #458681 - Flags: review?(sdwilsh)
Attachment #458757 - Flags: review?(sdwilsh)
PS: tryserver notified me of further test changes, so some more test change will be needed.
failure is still in test_bug_461710.html
requesting blocking because all of this stuff is redundant and blocks completing removal of temp tables (that should be a blocker itself)
blocking2.0: - → ?
Comment on attachment 458681 [details] [diff] [review]
wip patch

+++ b/docshell/test/chrome/bug293235_window.xul
-      // Because of LAZY_ADD, we will not be notified for three seconds
-      // Wait for four seconds just to be safe.
-      setTimeout(nextTest, 4000);
+      // Because adding visits is async, we will not be notified imemdiately.
+      setTimeout(nextTest, 1000);
Should we maybe change this to listen for the proper notification here?  Using a setTimeout seems risky.

+++ b/toolkit/components/places/src/AsyncFaviconHelpers.cpp
@@ -332,7 +332,7 @@ GetEffectivePageStep::HandleResult(mozIS
     rv = row->GetUTF8String(0, spec);
     FAVICONSTEP_FAIL_IF_FALSE_RV(NS_SUCCEEDED(rv), rv);
     // We always want to use the bookmark uri.
-    rv = mStepper->mPageURI->SetSpec(spec);
+    rv = NS_NewURI(getter_AddRefs(mStepper->mPageURI), spec);
How was this even OK before?  I don't see where we were even setting mStepper->mPageURI, so we should have been crashing, right?

+++ b/toolkit/components/places/src/nsNavHistory.cpp
@@ -5503,22 +5434,18 @@ nsNavHistory::SetPageTitle(nsIURI* aURI,
   if (aTitle.IsEmpty()) {
     nsString voidString;
     voidString.SetIsVoid(PR_TRUE);
+    rv = SetPageTitleInternal(aURI, voidString);
We should add a comment here as to why we use a void string (so it is stored as null in the db)

@@ -5793,18 +5720,6 @@ nsNavHistory::Observe(nsISupports *aSubj
     if (prefService)
       prefService->SavePrefFile(nsnull);
Huh...any idea why we do this?  This should happen already AFAIK during shutdown...

+++ b/toolkit/components/places/tests/browser/browser_bug399606.js
@@ -107,7 +105,7 @@ function test() {
       content.location.href = uri;
     }
     else {
-      setTimeout(confirm_results, LAZY_ADD_TIMER * 2.5);
+      setTimeout(confirm_results, 0);
At the very least here, we should be using executeSoon, but I think we really want to make this use an observer, right (more reliable + more likely to not be random orange).

+++ b/toolkit/components/places/tests/head_common.js
@@ -288,7 +288,6 @@ function check_no_bookmarks() {
 
 /**
  * Sets title synchronously for a page in moz_places synchronously.
- * History.SetPageTitle uses LAZY_ADD so we can't rely on it.
We should make this method use setPageTitle now since we can rely on it instead of duplicating code.

+++ b/toolkit/components/places/tests/mochitest/bug_411966/redirect.js
@@ -156,8 +156,7 @@ function checkDB(data){
   var referrer = this.mChannel.QueryInterface(Ci.nsIHttpChannel).referrer;
   ghist.addURI(this.mChannel.URI, true, true, referrer);
 
-  // We have to wait since we use lazy_add, lazy_timer is 3s
-  setTimeout("checkDBOnTimeout()", 4000);
+  setTimeout(checkDBOnTimeout, 0);
We do not need the timeout here at all.  I don't even think this test needs to query the database directly anymore.

+++ b/toolkit/components/places/tests/mochitest/test_bug_461710.html
@@ -98,9 +101,12 @@ function loadNextTest() {
-    // Because of LAZY_ADD, the page won't be marked as visited until three seconds,
-    // so wait for four seconds to be safe
-    setTimeout(handleLoad, LAZY_ADD_TIMER * 2);
Ha!  This comment was so wrong...

+++ b/toolkit/components/places/tests/unit/test_404630.js
   try {
-    faviconService.setAndLoadFaviconForPage(uri("http://www.mozilla.com"), null, false);
+    PlacesUtils.favicons.setAndLoadFaviconForPage(
+      null, uri("http://www.mozilla.com/favicon.ico"), false
+    );
+    do_throw("should throw because page param is null");
+  } catch (ex) {
+    do_check_true(true);
Would prefer you have a variable like |exceptionCaught| that you set to false at first, and then set to true in the catch.  Then outside, check that it is true.

+  try {
+    PlacesUtils.favicons.setAndLoadFaviconForPage(
+      uri("http://www.mozilla.com"), null, false
+    );
     do_throw("should throw because favicon param is null");
-  } catch (ex) {}
+  } catch (ex) {
+    do_check_true(true);
+  }
ditto here

r=sdwilsh with comments addressed
Attachment #458681 - Flags: review?(sdwilsh) → review+
Comment on attachment 458757 [details] [diff] [review]
additional tests fixes on top of wip patch

r=sdwilsh
Attachment #458757 - Flags: review?(sdwilsh) → review+
(In reply to comment #9)
> +++ b/toolkit/components/places/src/AsyncFaviconHelpers.cpp
> @@ -332,7 +332,7 @@ GetEffectivePageStep::HandleResult(mozIS
>      rv = row->GetUTF8String(0, spec);
>      FAVICONSTEP_FAIL_IF_FALSE_RV(NS_SUCCEEDED(rv), rv);
>      // We always want to use the bookmark uri.
> -    rv = mStepper->mPageURI->SetSpec(spec);
> +    rv = NS_NewURI(getter_AddRefs(mStepper->mPageURI), spec);
> How was this even OK before?  I don't see where we were even setting
> mStepper->mPageURI, so we should have been crashing, right?

Actually mPageURI exists, but mMutable is false here now, it was not before, I guess lazy_add was making us act later when that was not a problem. I don't know nsIURI internals enough to figure out all cases where mMutable is set to false.

(In reply to comment #9)
> @@ -5793,18 +5720,6 @@ nsNavHistory::Observe(nsISupports *aSubj
>      if (prefService)
>        prefService->SavePrefFile(nsnull);
> Huh...any idea why we do this?  This should happen already AFAIK during
> shutdown...

IIRC I did tests in the past and it was not saved without this piece, I think it was due to the fact we were shutting down at xpcom-shutdown. Now that we shutdown earlier it should not be an issue. But I'd prefer not touching it here.
addressed comments, all tests passing locally and now building on tryserver
Attachment #458681 - Attachment is obsolete: true
Attachment #458757 - Attachment is obsolete: true
Comment on attachment 459025 [details] [diff] [review]
rolled up patch v1.0

tryserver gave greeen, this is ready to land, low risk since is mostly code removal, with test fixes.
Attachment #459025 - Flags: approval2.0?
Comment on attachment 459025 [details] [diff] [review]
rolled up patch v1.0

a=beltzner, someone should still come by and make the blocking decision, though, to ensure we catch it if it regresses/gets reopened.
Attachment #459025 - Flags: approval2.0? → approval2.0+
thanks
http://hg.mozilla.org/mozilla-central/rev/87fddd376838
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
blocking+, due to blocking of bug 552023, which blocks bug 563538, which is a blocker.
blocking2.0: ? → betaN+
Depends on: 943148
You need to log in before you can comment on or make changes to this bug.