735532 - Form history bulk clear events not tracked

Status: RESOLVED DUPLICATE of bug 1404427

(Firefox :: Sync, defect)

Description

[Nick Alexander :nalexander [he/him]]

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.)

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: First log showing record propagating to server.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Second log showing deletion not propagating to server

Comment 3

[Richard Newman [:rnewman]]

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);

Comment 4

[Philipp von Weitershausen [:philikon]]

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

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: Trace log showing initial propagation to server

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Trace log showing failure to propagate delete to server

Comment 7

[Nick Alexander :nalexander [he/him]]

I see nothing in the trace logs about FormsTracker (maybe I'm looking in the wrong place) and I see no deletes propagating.

Comment 8

[Richard Newman [:rnewman]]

(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.

Comment 9

[Richard Newman [:rnewman]]

Attachment: 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

Comment 10

[Tracy Walker [:tracy]]

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.

Comment 12

[Gregory Szorc [:gps]]

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?

Comment 14

[Mark Hammond [:markh] [:mhammond]]

To try and ensure this gets some attention, I'm blocking our generic "sync moar things" bug...

Comment 15

[github]

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.

Comment 16

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 17

[Lina Butler [:lina]]

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.