Form history bulk clear events not tracked

RESOLVED DUPLICATE of bug 1404427

Status

()

RESOLVED DUPLICATE of bug 1404427
7 years ago
3 months ago

People

(Reporter: nalexander, Unassigned)

Tracking

(Blocks: 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sync:forms])

Attachments

(5 attachments)

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.)
Created attachment 605609 [details]
First log showing record propagating to server.
Created attachment 605610 [details]
Second log showing deletion not propagating to server
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
Created attachment 605616 [details]
Trace log showing initial propagation to server
Created attachment 605617 [details]
Trace log showing failure to propagate delete to server
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.
Created attachment 605648 [details] [diff] [review]
WIP. v1

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

Updated

3 years ago
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: 1269548

Comment 15

2 years ago
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.
Depends on: 1346850
Blocks: 1381372
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: → bug 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
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1404427
(Assignee)

Updated

3 months ago
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.