Closed
Bug 1404427
Opened 7 years ago
Closed 7 years ago
Sync mass Form History deletions
Categories
(Firefox :: Sync, enhancement, P1)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: eoger, Assigned: eoger)
References
(Blocks 1 open bug)
Details
(Whiteboard: [Sync Q4 OKRs])
Attachments
(1 file)
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.
Comment 1•7 years ago
|
||
(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.
Comment 2•7 years ago
|
||
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
Updated•7 years ago
|
Priority: -- → P2
Whiteboard: [Sync Q4 OKRs]
Comment 3•7 years ago
|
||
(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.
Updated•7 years ago
|
Flags: needinfo?(kit)
Comment 5•7 years ago
|
||
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)
Updated•7 years ago
|
Assignee: nobody → eoger
Assignee | ||
Updated•7 years ago
|
Priority: P2 → P1
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
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 10•7 years ago
|
||
mozreview-review |
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+
Comment 11•7 years ago
|
||
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6aa22b862a45
Sync multiple form history deletions. r=kitcambridge
Updated•7 years ago
|
Flags: needinfo?(eoger)
Comment 12•7 years ago
|
||
Backed out changeset 6aa22b862a45 (bug 1404427) for failing browser-chrome browser/components/search/test/browser_426329.js r=backout on a CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/7e046d1623948c67b23d56bc9fa19f0274bb686b
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6aa22b862a45b7db9a09175d2b939c4a79a80de7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable
https://treeherder.mozilla.org/logviewer.html#?job_id=143386050&repo=autoland&lineNumber=5570
Log snippet:
TEST-UNEXPECTED-FAIL | browser/components/search/test/browser_426329.js | History cleared -
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(eoger)
Comment 14•7 years ago
|
||
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9a7e2fba868
Sync multiple form history deletions. r=kitcambridge
Comment 15•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 16•7 years ago
|
||
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 → ---
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e02d99be7eb0
Sync multiple form history deletions. r=kitcambridge
Comment 20•7 years ago
|
||
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 21•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment 24•7 years ago
|
||
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24cdbe1d8af0
Sync multiple form history deletions. r=kitcambridge
Comment 25•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(eoger)
Updated•7 years ago
|
relnote-firefox:
--- → ?
Comment 26•7 years ago
|
||
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.
Comment 28•7 years ago
|
||
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.
Description
•