TPS form tests aren't doing what they think they are doing causing validation failures.

RESOLVED FIXED in Firefox 52

Status

Testing
TPS
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: markh, Assigned: markh)

Tracking

Version 3
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

I noticed some form validation errors in TPS, which Thom mentioned in bug 1286923, but apparently the issue was intermittent for him. So I looked into the failures and found lots of things wrong. Of note:

* The test was inserting form data directly into the DB. While this is somewhat fragile, the 2 bigger problems with this are:
** The notifications sent by the "real" form manager aren't sent, so the tracker never sees changes.
** It directly inserts into the DB even in private-browsing mode. However, private-browsing mode skips a database update (eg, see [1])
** The test never actually tried to Sync these newly inserted records - so all it was doing in practice was checking the rows it wrote were actually written.

To fix this I:
* Rewrote the form database code to use the (promise-based) FormHistory.jsm module.
* Deleted the private-browsing test entirely. The only way to make it sane would be to "organically" populate real forms on real sites, and even then, it's not really a Sync test - Sync doesn't care where the entries came from. If they end up getting Synced it's a bug somewhere other than Sync.
* For completeness, changes the non-pb test to add new entries after a first-sync, which means we rely on the tracker seeing the notification.

A couple of other tweaks in this patch that I didn't think was worth their own bug:
* We always warn "Changed IDs file {filename} contains non-object value." when the file simply doesn't exist - now we don't warn.
* One case where we just did |thisArg._log.debug("Exception", ex);| now logs the function name that failed.
* In TPS we shut down the logger in the "quit-application-requested" notification handler, but Sync keeps doing stuff after that, so I now use "profile-before-change" to ensure we get as many log messages as possible.
* The sync signout didn't wait for the signout to complete, meaning lots of exceptions were logged as the wipeServer etc cleanup calls were terminated due to a shutdown exception. I fixed this by waiting for the signout to complete - and waitForEvent wasn't quite suitable as the event might be fired before TPS called waitForEvent. Hopefully that change is obvious.

Let me know if you prefer I split those other patches into a different bug. Patch forthcoming.

[1] https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/toolkit/components/satchel/formSubmitListener.js#107
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
mozreview-review
Comment on attachment 8792784 [details]
Bug 1303945 - Avoid having TPS write directly to the form history database and remove private-browsing tests.

https://reviewboard.mozilla.org/r/79702/#review78468

::: services/sync/tps/extensions/tps/resource/tps.jsm:995
(Diff revision 2)
>     * returning.
>     *
>     * @param aEventName
>     *        String event to wait for.
>     */
>    waitForEvent: function waitForEvent(aEventName) {

I should reimplement this as createEventWaiter()()

Comment 4

a year ago
mozreview-review
Comment on attachment 8792784 [details]
Bug 1303945 - Avoid having TPS write directly to the form history database and remove private-browsing tests.

https://reviewboard.mozilla.org/r/79702/#review78526

Looks great!  Thanks for digging into this, and also for fixing the other errors. Don't worry about splitting into multiple patches.

::: services/sync/tps/extensions/tps/resource/tps.jsm:995
(Diff revision 2)
>     * returning.
>     *
>     * @param aEventName
>     *        String event to wait for.
>     */
>    waitForEvent: function waitForEvent(aEventName) {

Your call, I don't care either way. Might want to adjust the comment on createEventWaiter if you do, although I guess it would still apply as written.

::: services/sync/tps/extensions/tps/resource/tps.jsm:1029
(Diff revision 2)
> +    Logger.logInfo("Setting up wait for " + aEventName + "...");
> +    let cb = Async.makeSpinningCallback();
> +    Svc.Obs.add(aEventName, cb);
> +    return function() {
> +      cb.wait();
> +      Svc.Obs.remove(aEventName, cb);

Should this be in a finally? I'm not sure how bad not removing this observer would be -- honestly it probably doesn't matter for TPS, my concern I guess is more if it got moved or copypasted elsewhere.

Your call either way though, it doesn't matter that much to me.
Attachment #8792784 - Flags: review?(tchiovoloni) → review+
Comment hidden (mozreview-request)

Comment 6

a year ago
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/ce23198d0d58
Avoid having TS write directly to the form history database and remove private-browsing tests. r=tcsc
My log change broke a test. Tomcat backed it out for me.
Comment hidden (mozreview-request)

Comment 9

a year ago
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/7556f58fa06d
Avoid having TPS write directly to the form history database and remove private-browsing tests. r=tcsc

Comment 10

a year ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e57d63c8fd71
Backed out changeset ce23198d0d58 on request from mark
To be clear: comment 10 is the backout of the earlier push in comment 6. Comment 9 is a new push

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7556f58fa06d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.