Closed Bug 735532 Opened 9 years ago Closed 3 years ago

Form history bulk clear events not tracked

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1404427

People

(Reporter: nalexander, Unassigned)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [sync:forms])

Attachments

(5 files)

STR:

Navigate to site with a form field (I have been using www.ryerson.ca/JavaScript/lectures/forms/textinput.html).
Enter data.
Refresh page; observer form history correctly remembered locally.
Sync.
Witness first log showing form history propagating to server.

Either clear form history in preferences or delete form history item using https://addons.mozilla.org/en-US/firefox/addon/form-history-control/.
Observe form history data correctly forgotten locally.
Sync.
Witness second log showing no form history changes propagating to server.
(I have also checked with in-development extensions to Android Sync and see no form history records marked as deleted being downloaded.)
Just as with Bug 578694, we don't act on the appropriate notifications.

      case "satchel-storage-changed":
        if (data == "addEntry" || data == "before-removeEntry") {

vs.

    removeAllEntries : function () {
        this.log("removeAllEntries");

        this.sendNotification("before-removeAllEntries", null);

and

    removeEntriesForName : function (name) {
        this.log("removeEntriesForName with name=" + name);

        this.sendStringNotification("before-removeEntriesForName", name);
OS: Mac OS X → All
Hardware: x86 → All
Summary: Desktop Sync not propagating form history deleted item to server → Form history clear events not tracked
Nick, please enable trace logging for the forms engine and retry your experiment. You should see the FormsTracker tracking the form history entries as you remove them, and then sync them. Might also be a good idea to add some more logging information here: https://mxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/forms.js#321
I see nothing in the trace logs about FormsTracker (maybe I'm looking in the wrong place) and I see no deletes propagating.
(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> Nick, please enable trace logging for the forms engine and retry your
> experiment. You should see the FormsTracker tracking the form history
> entries as you remove them, and then sync them.

nsFormHistory.removeAllEntries does not notify before-removeEntry for each item. It sends before-removeAllEntries, then clears the DB, then notifies removeAllEntries.

Sync isn't watching for bulk deletes.
Attached patch WIP. v1Splinter Review
This handles removeAllEntries the hard way.

There is a much more efficient approach. And I only stubbed out the other possible deletion pathways. But this seems to work.

1331697607251	Sync.Tracker.Forms	DEBUG	Notified of removeAllEntries.
...
1331697607687	Sync.Engine.Forms	INFO	1 outgoing items pre-reconciliation
1331697607688	Sync.Engine.Forms	INFO	Records: 0 applied, 0 successfully, 0 failed to apply, 0 newly failed to apply, 0 reconciled.
1331697607692	Sync.Engine.Forms	INFO	Uploading all of 1 records
1331697607693	Sync.Collection	DEBUG	POST Length: 253
Attachment #605648 - Flags: feedback?(gps)
fyi on STR's  you can clear any single form history item by highlighting it in the actual form field drop down list, then click option(alt) + shift + delete.

Will dupe bug I filed on this nearly 2 years ago.
Duplicate of this bug: 564296
Comment on attachment 605648 [details] [diff] [review]
WIP. v1

Review of attachment 605648 [details] [diff] [review]:
-----------------------------------------------------------------

I'm cancelling the feedback because 1) there is lots more to do (including possibly a more proper solution per your comment) and 2) I don't fully grok the API yet. I'll happily review something that's more complete.

::: services/sync/modules/engines/forms.js
@@ +308,5 @@
>      this.score += SCORE_INCREMENT_MEDIUM;
>    },
>  
>    _enabled: false,
>    observe: function observe(subject, topic, data) {

Uncaught exceptions in observers scare me.

@@ +364,5 @@
> +        if (data == "before-removeEntriesByTimeframe") {
> +          subject = subject.QueryInterface(Ci.nsIArray);
> +
> +          // Timestamps in microseconds.
> +          let begin = subject.queryElementAt(0, Ci.nsISupportsPRInt64).data;

I'm not entirely sure if queryElementAt will throw (like QueryInterface does) if the QI is not valid. I would assume so. However, https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIArray#queryElementAt%28%29 also says that the return may be null. Do we need to guard against null access?
Attachment #605648 - Flags: feedback?(gps)
Whiteboard: [sync:forms]
Blocks: 833044
Blocks: 1226369
Duplicate of this bug: 1329031
To try and ensure this gets some attention, I'm blocking our generic "sync moar things" bug...
Blocks: syncmore
Thanks, Mark. One could argue that this is a bit of a security risk as it's easy to mistype a password in a username field. With this bug, you can't get rid of it. If someone is standing over your shoulder, they can see the password briefly in the dropdown of previously entered values.
No longer blocks: 1381372
Note that single deletions (eg, using the "delete" key on the popup) is being done in bug 1395427, so this bug remains for the various "bulk" operations.
See Also: → 1395427
Summary: Form history clear events not tracked → Form history bulk clear events not tracked
I think we're going to go with a different approach in bug 1404427, storing tombstones for cleared entries (or, alternatively, date ranges) in a separate SQL table instead of relying on observers.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1404427
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.