Closed Bug 1404427 Opened 7 years ago Closed 7 years ago

Sync mass Form History deletions

Categories

(Firefox :: Sync, enhancement, P1)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Sync Q4 OKRs])

Attachments

(1 file)

Bug 1404427 - Sync multiple form history deletions.
59 bytes, text/x-review-board-request
lina
: review+
Details 
Currently we don't sync form history deletions when we use the Forget/Clear Recent History buttons.
The naive solution would be to execute a SELECT statement to retrieve the GUIDs we are about to delete and pass these in the "formhistory-remove" observer notification.

What we could do instead is move these GUIDs to a new table moz_syncdeletions(guid, engine, sync_status).
When syncing, we would upload these tombstones if necessary and bump the sync_status counter accordingly.
This would allow us to keep tombstones locally to avoid resurrections, and we could also use that table for other engines like history.
(In reply to Edouard Oger [:eoger] from comment #0)
> What we could do instead is move these GUIDs to a new table
> moz_syncdeletions(guid, engine, sync_status).
> When syncing, we would upload these tombstones if necessary and bump the
> sync_status counter accordingly.

This sounds like a solid plan! Store downloaded tombstones as NORMAL, local tombstones as NEW, flip NEW to NORMAL after upload, clear on sign out/node reassignment/engine disable. I think form history lives in a separate `formhistory.sqlite` database, so we'd need to attach to Places if we wanted to use the same table for history tombstones. That's fine; though, the interest of simplicity I'd suggest separate tables for now.

We may want to give some thought to having a separate representation for bulk deletes on the server. I don't think there's a problem with storing thousands of rows in a tombstone table locally: SQLite handles that well, and we could do the insert in a single statement.

Uploading, storing, and downloading thousands of tombstone records concerns me a bit...but we can already store hundreds of thousands of form history entries on the server, *without* clearing any of them, so I don't think we'll be any worse off. We might also want to think about evicting old tombstones (on both sides) eventually, but that doesn't need to be part of this bug.
See Bug 833044.

N.B., Sync has a `wipeEngine` command to try to address this case. Naturally it doesn't work for clients that are offline for a long time, disconnect then reconnect, etc.

N.B.B., we set a 3-year TTL on form history records, so they don't live quite forever.
Blocks: 833044
Priority: -- → P2
Whiteboard: [Sync Q4 OKRs]
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #1)
> Uploading, storing, and downloading thousands of tombstone records concerns
> me a bit...but we can already store hundreds of thousands of form history
> entries on the server, *without* clearing any of them, so I don't think
> we'll be any worse off. We might also want to think about evicting old
> tombstones (on both sides) eventually, but that doesn't need to be part of
> this bug.

I think this will be tricky. We already limit the amount of history we sync and don't care about older history we miss. Will we limit the tombstones we Sync similarly? If not, and given enough time, there will be a huge number of tombstones on a first sync. If so, will that defeat the intent of this bug?

The fact we limit history sucks IMO. I wonder if we might need some kind of "incremental" process, both for upload and download? We appear to have attempted support for downloads at http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/services/sync/modules/engines.js#1258 (but it's not clear to me that it works - it certainly has no test coverage), but IIUC, we have no such concept for uploads.
Flags: needinfo?(kit)
Added some rambling thoughts to bug 578694 for Places history. We could try the same approach for form history, or, for expediency, sync per-record tombstones. I imagine (citation needed) these wouldn't churn as much as visits...
Flags: needinfo?(kit)
Assignee: nobody → eoger
Priority: P2 → P1
Comment on attachment 8925657 [details]
Bug 1404427 - Sync multiple form history deletions.

https://reviewboard.mozilla.org/r/196792/#review202458

Thanks, Ed! I'd like to see the patch again with comments addressed, but this is looking great.

::: toolkit/components/satchel/FormHistory.jsm:654
(Diff revision 1)
>            delete change.timeDeleted;
>          }
>          stmt = makeRemoveStatement(change, bindingArrays);
> -        notifications.push(["formhistory-remove", change.guid]);
> +
> +        // Fetch the GUIDs we are going to delete.
> +        await new Promise((res, rej) => {

Let's make this a `try...catch`, and log inside the `catch` block.

::: toolkit/components/satchel/FormHistory.jsm:654
(Diff revision 1)
>            delete change.timeDeleted;
>          }
>          stmt = makeRemoveStatement(change, bindingArrays);
> -        notifications.push(["formhistory-remove", change.guid]);
> +
> +        // Fetch the GUIDs we are going to delete.
> +        await new Promise((res, rej) => {

It *almost* seems like `update` would be a better place to do this, since it already calls `search`, bumps `numSearches`, and waits to call `updateFormHistoryWrite`. But it's not quite what we want, and I think your approach here is less invasive than trying to change `update` to do what we want.

::: toolkit/components/satchel/FormHistory.jsm:656
(Diff revision 1)
>          stmt = makeRemoveStatement(change, bindingArrays);
> -        notifications.push(["formhistory-remove", change.guid]);
> +
> +        // Fetch the GUIDs we are going to delete.
> +        await new Promise((res, rej) => {
> +          let bindingArrays = new Map();
> +          let selectStmt = makeRemoveStatement(change, bindingArrays, true);

How do you feel about using `makeSearchStatement(change, ["guid"])` here, instead of changing `makeRemoveStatement` to take a Boolean, or using a separate function for this? (Bonus: IIUC, we can skip binding parameters in that case, too).

I'm not a fan of Boolean arguments in general, and this one in particular changes the function's behavior from "delete and return nothing" to "fetch and return things".

::: toolkit/components/satchel/FormHistory.jsm:662
(Diff revision 1)
> +          selectStmt.bindParameters(bindingArrays.get(selectStmt));
> +          let selectHandlers = {
> +            handleCompletion() {
> +              res();
> +            },
> +            handleError() {

Let's log errors, but not forward them to `aCallbacks`. It's not fatal if we can't notify observers, and we don't want callers like https://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/browser/base/content/sanitize.js#438,451 transforming these into exceptions.

::: toolkit/components/satchel/test/unit/test_notify.js:189
(Diff revision 1)
>      testdesc = "removeEntriesByTimeframe";
>  
>      expectedNotification = "formhistory-remove";
> -    expectedData = [10, 99999999999];
> +    expectedData = [10, Date.now() * 1000 + 1];
> +    subjectIsGuid = true;
>  

It'd be good to verify we get the expected number of observer notifications here, too.
Attachment #8925657 - Flags: review?(kit)
Comment on attachment 8925657 [details]
Bug 1404427 - Sync multiple form history deletions.

https://reviewboard.mozilla.org/r/196792/#review202458

> It'd be good to verify we get the expected number of observer notifications here, too.

Thanks, I ended up refactoring the whole test.
Comment on attachment 8925657 [details]
Bug 1404427 - Sync multiple form history deletions.

https://reviewboard.mozilla.org/r/196792/#review203096

Awesome, thanks! \o/ I didn't mean to make you slog through rewriting the test, but it's so much easier to follow now.
Attachment #8925657 - Flags: review?(kit) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6aa22b862a45
Sync multiple form history deletions. r=kitcambridge
Flags: needinfo?(eoger)
Flags: needinfo?(eoger)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9a7e2fba868
Sync multiple form history deletions. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/d9a7e2fba868
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Backed out on suspicion of causing failures in browser_sanitize-timespans.js at some times of the day:

https://hg.mozilla.org/mozilla-central/rev/0e18448c495cb5b3da689a554e8137dbd8eddcc0

See this push range, you will have to check the time (and data center? UTC?) the jobs run: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=4be8afd896458fae76dc2eacf813a5eaa26db483&filter-searchStr=windows%20debug%20browser-chrome&tochange=5f3ad5cd80602d9d2b9e912be92776b445a8332a&selectedJob=143507077

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=143507077&repo=autoland
00:46:37     INFO -  575 INFO TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | <4 hour old download should now be deleted -
00:46:37     INFO -  576 INFO TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | 4 hour 10 minute download should still be present -
00:46:37     INFO -  577 INFO TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | Year old download should still be present -
00:46:37     INFO -  578 INFO TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | Pretend visit to 4hour10minutes.com should now be deleted -
00:46:37     INFO -  579 INFO TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | Pretend visit to before-today.com should still exist -
00:46:37     INFO -  580 INFO TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | 4hour10minutes form entry should be deleted -
00:46:37     INFO -  581 INFO TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | b4today form entry should still exist -
00:46:37     INFO -  582 INFO TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | 4 hour 10 minute download should now be deleted -
00:46:37     INFO -  583 INFO TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | Year old download should still be present -
00:46:37     INFO -  Buffered messages logged at 00:45:07
00:46:37     INFO -  584 INFO Longer timeout required, waiting longer...  Remaining timeouts: 1
00:46:37     INFO -  Buffered messages finished
00:46:37    ERROR -  585 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_sanitize-timespans.js | Test timed out -

Last PASS statement is from https://hg.mozilla.org/mozilla-central/annotate/2535bad09d720e71a982f3f70dd6925f66ab8ec7/browser/base/content/test/general/browser_sanitize-timespans.js#l390
Status: RESOLVED → REOPENED
status-firefox58: fixed → ---
Flags: needinfo?(eoger)
Resolution: FIXED → ---
Target Milestone: Firefox 58 → ---
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e02d99be7eb0
Sync multiple form history deletions. r=kitcambridge
Backed out for browser failures in browser_sanitize-timespans.js at some times of the day.
https://hg.mozilla.org/integration/autoland/rev/9c83fd7c7c7a12fedd669af3eda5b679936d15b8

Push with failure: 
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e02d99be7eb02399c057b77d3b1c9c6cda054ef5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=257be0b37efc11b1d11f94b719c5c368861fcbe3&selectedJob=143906002

Failure log:
https://treeherder.mozilla.org/logviewer.html#?job_id=143906002&repo=autoland&lineNumber=3523

00:10:08    ERROR -  672 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_sanitize-timespans.js | Test timed out -
00:10:08     INFO -  GECKO(4472) | MEMORY STAT | vsize 991MB | vsizeMaxContiguous 494MB | residentFast 132MB | heapAllocated 84MB
00:10:08     INFO -  673 INFO TEST-OK | browser/base/content/test/general/browser_sanitize-timespans.js | took 225030ms
00:10:08     INFO -  674 INFO checking window state
00:10:08     INFO -  675 INFO TEST-START | browser/base/content/test/general/browser_sanitizeDialog.js
00:10:08     INFO -  Not taking screenshot here: see the one that was previously logged
00:10:08     INFO -  Buffered messages logged at 00:10:08
00:10:08     INFO -  676 INFO Entering test bound init
00:10:08     INFO -  Buffered messages finished
00:10:08    ERROR -  677 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_sanitize-timespans.js | Uncaught exception received from previously timed out test - at chrome://mochitests/content/browser/browser/base/content/test/general/browser_sanitize-timespans.js:417 - ReferenceError: ok is not defined
00:10:08     INFO -  Stack trace:
00:10:08     INFO -      onHistoryReady@chrome://mochitests/content/browser/browser/base/content/test/general/browser_sanitize-timespans.js:417:5
00:10:08     INFO -      async*test@chrome://mochitests/content/browser/browser/base/content/test/general/browser_sanitize-timespans.js:49:9
00:10:08     INFO -      Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1060:21
00:10:08     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:1051:9
00:10:08     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:951:9
00:10:08     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
Comment on attachment 8925657 [details]
Bug 1404427 - Sync multiple form history deletions.

https://reviewboard.mozilla.org/r/196792/#review203924

::: toolkit/components/satchel/FormHistory.jsm:665
(Diff revision 4)
> +              handleError() {
> +                log("remove select guids failure");
> +              },
> +              handleResult(aResultSet) {
> +                for (let row = aResultSet.getNextRow(); row; row = aResultSet.getNextRow()) {
> +                  notifications.push(["formhistory-remove", row.getResultByName("guid")]);

Sorry, I think there's a subtle behavior change that I missed in my review. Before, we'd always notify `satchel-storage-changed`, even if we didn't actually remove anything; now, we'll only notify if we found some GUIDs to remove. I wonder if the failing test relies on the old behavior.
Comment on attachment 8925657 [details]
Bug 1404427 - Sync multiple form history deletions.

https://reviewboard.mozilla.org/r/196792/#review203924

> Sorry, I think there's a subtle behavior change that I missed in my review. Before, we'd always notify `satchel-storage-changed`, even if we didn't actually remove anything; now, we'll only notify if we found some GUIDs to remove. I wonder if the failing test relies on the old behavior.

Indeed! I found the blocker by changing the Date on my computer and running the test again.
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24cdbe1d8af0
Sync multiple form history deletions. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/24cdbe1d8af0
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Flags: needinfo?(eoger)
I did some verification between FF60 clients using Clear History as well as the Forget button. I also did some between FF60 & FF58 clients. History deletion followed by a sync deleted history appropriately.
What's the ask for release notes here?
Flags: needinfo?(kit)
Oops, I think we decided against mentioning this in release notes, since this is more "Sync now does what you expected it to do all along". Clearing relnote? flag.
relnote-firefox: ? → ---
Flags: needinfo?(kit)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: