Would be cool if we could throw away LAZY_ADD and use async storage.

Insofar as this causes bug 550778, this should probably block 1.9.3.

Doesn't seem like this blocks by itself. Bug 550778 is the blocker, if this fixes it then awesome.

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.

additional fixes, since Shawn is reviewing the first wip patch, this is like an interdiff, will merge patches after review.

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)

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

Comment on attachment 458757 [details] [diff] [review] additional tests fixes on top of wip patch r=sdwilsh

(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

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.

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.

thanks http://hg.mozilla.org/mozilla-central/rev/87fddd376838

blocking+, due to blocking of bug 552023, which blocks bug 563538, which is a blocker.