Ensure places/origins are removed when a bookmark with no visits etc is removed.

[mhammond]

Fixes a few reports we have on Fenix for this, such as this one and I'm sure other dupes exist.

Lina, sorry to do this to you, but you are probably the best reviewer for this :( I'll also ask Ben to help grow more knowledge about places. This is by no means urgent.

[codecov-commenter]

Codecov Report

Patch and project coverage have no change.

Comparison is base (e0b1d5b) 84.02% compared to head (482f1f9) 84.02%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5653   +/-   ##
=======================================
  Coverage   84.02%   84.02%           
=======================================
  Files         106      106           
  Lines       11272    11272           
=======================================
  Hits         9471     9471           
  Misses       1801     1801           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[linabutler]

I'm also confused about why this trigger works this way!

Desktop's moz_places_afterinsert_trigger doesn't update moz_origins directly; it inserts rows into moz_updateoriginsinsert_temp, and does the actual updates to moz_origins inside moz_updateoriginsinsert_afterdelete_trigger, like you noticed: https://searchfox.org/mozilla-central/rev/962a843f6d96283c45162c788dc72bf217fca7df/toolkit/components/places/nsPlacesTriggers.h#104-139

IOW, Desktop's moz_places_afterinsert_trigger looks identical to our moz_places_afterinsert_trigger_origins.

I did some spelunking, and found https://bugzilla.mozilla.org/show_bug.cgi?id=1414892. It looks like that's when the TEMP table was introduced—except, back then, the table was still called moz_hosts, not moz_origins.

I'm also unsure if the "expected" frecency of 1999 is correct. I might have changed -1 to 1999 in c27b45f, because the important part was that it wasn't -1, not that it was specifically 1999. But 0 doesn't sound right, either...

I stood up an xpcshell test in Desktop, and it reports a frecency of 2000 for a freshly inserted typed visit—but Desktop's algorithm has changed a few times since we ported it to Application Services. Building an early-2018 Firefox, when we ported history, is definitely a challenge 😅

[linabutler]

@mhammond I figured it out—the order of the deletes in delete_pending_temp_tables is off! 🤯

I think the order of those deletes has to be:

DELETE FROM moz_updateoriginsinsert_temp;
DELETE FROM moz_updateoriginsupdate_temp;
DELETE FROM moz_updateoriginsdelete_temp;

Specifically, it's important that DELETE FROM moz_updateoriginsinsert_temp runs before DELETE FROM moz_updateoriginsupdate_temp. Here's what I think is happening, and where the bug is:

When apply_observation_direct adds visits for a never-before-visited URL, it does three things:

1. Inserts a row into moz_places for the new URL.
2. Inserts rows into moz_historyvisits for the new visits.
3. Updates moz_places.frecency for the new URL to account for the visits.

(1) fires the moz_places_afterinsert_trigger_origins trigger, and (3) fires the moz_places_afterupdate_frecency_trigger.

Currently, delete_pending_temp_tables does DELETE FROM moz_updateoriginsupdate_temp first. That'll fire the moz_updateoriginsupdate_afterdelete_trigger, which will try to update moz_origins.frecency for the new URL's origin.

But, there isn't a row for the new URL in moz_origins yet—because that row is inserted by the moz_updateoriginsinsert_afterdelete_trigger, and that trigger won't fire until we do DELETE FROM moz_updateoriginsinsert_temp!

So this UPDATE moz_origins statement won't do anything, and the initial frecency for the new URL's origin—2000, same as Desktop—will be silently dropped.

When DELETE FROM moz_updateoriginsinsert_temp eventually runs, it'll insert a row for the new URL's origin (via the moz_updateoriginsinsert_afterdelete_trigger), but with an initial moz_origins.frecency of 0.

Desktop's behavior is correct here, here, and here. It always processes inserts for new origins before frecency updates for those origins.

With the order of the deletes in delete_pending_temp_tables fixed, the frecency for the test site's origin in matcher.rs also changes to match Desktop's! 🎉

TL;DR: I think there are three small fixes:

1. Change the order of the deletes in delete_pending_temp_tables.
2. Delete the moz_places_afterinsert_trigger.
3. Change the expected frecency in the api::matcher::tests::search test from 1999 to 2000.

[mhammond]

ah yes, thank you! I actually started with a theory that the order of deletions was wrong, but then convinced myself I was confused about that.

[linabutler]

Thank you so much for looking at this! 🏆

I wouldn't mind having a fresh look with the origin frecency trigger changes, but your new moz_cleanup_origin_bookmark_deleted_trigger and tests look great!

[linabutler]

I wonder if we should do the equivalent of delete_pending_temp_tables here—or at least a DELETE FROM moz_updateoriginsdelete_temp—after deleting the place.

I think that would mean the origin would get cleaned up as soon as the bookmark is deleted, and you could avoid the call to crate::storage::history::delete_everything(&conn)? in the bookmarks test.

WDYT?

[mhammond]

I don't quite understand this suggestion. First of all though, you are correct that the new call I added to delete_everything can be removed immediately.

But ISTM that the new trigger already is forcing that delete_pending_temp_tables dance - the trigger on moz_bookmarks causes a deletion of moz_places, which causes the moz_places_afterdelete_trigger_origins trigger. So sorry for being dense, but do you mind spelling it out :)

[linabutler]

Oh, rad!

the trigger on moz_bookmarks causes a deletion of moz_places, which causes the moz_places_afterdelete_trigger_origins trigger.

Yep, you're right!

But the moz_places_afterdelete_trigger_origins trigger just inserts a row for the soon-to-be-deleted origin into moz_updateoriginsdelete_temp. It doesn't actually delete the origin from moz_origins, or update the global frecency stats in moz_meta, until you do DELETE FROM moz_updateoriginsdelete_temp to fire the moz_updateoriginsdelete_afterdelete_trigger.

At first, I thought this would leave the origin behind, because doing DELETE FROM moz_places by itself isn't enough to force the delete_pending_temp_tables dance. And that's why I thought we needed to force it by doing a DELETE FROM moz_updateoriginsdelete_temp in your new trigger.

But then I realized that delete_bookmark calls delete_pending_temp_tables! So it's not necessary to do it in your new trigger, because the delete_pending_temp_tables call here will take care of it.

So this is all working correctly now—if in a slightly indirect way that I didn't think all the way through—and you can ignore my suggestion! 😅

[mhammond]

New version up with all suggested changes except the one I don't understand :)

[linabutler]

Thanks! My comment was totally a non-issue, sorry to muddle things there 🙈 Land away!