1383622 - Remove use of sync bookmarks API from TPS

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P3)

Description

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

TPS still uses the sync bookmarks API - we should convert it to use the async API

Comment 1

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

Attachment: Bug 1383622 (part 1) - convert most of TPS to async/await.

Review commit: https://reviewboard.mozilla.org/r/168538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168538/

Comment 2

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

Attachment: Bug 1383622 (part 2) - update TPS to use the async bookmark API.

Review commit: https://reviewboard.mozilla.org/r/168540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168540/

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8897254 [details]
Bug 1383622 (part 1) - convert most of TPS to async/await.

https://reviewboard.mozilla.org/r/168538/#review173796

This has been a long time coming, glad to see it happening.

::: services/sync/tps/extensions/tps/resource/tps.jsm:283
(Diff revision 1)
>        this._operations_pending--;
>      }
>      if (!this.operations_pending) {
>        this._currentAction++;
>        Utils.nextTick(function() {
> -        this.RunNextTestAction();
> +        this.RunNextTestAction().catch(err => {this.DumpError("RunNextTestActionFailed", err)});;

nit: Can you space this out with whitespace or newlines?

::: services/sync/tps/extensions/tps/resource/tps.jsm:1093
(Diff revision 1)
> -   * Synchronously wait for the named event to be observed.
> +   * Wait for the named event to be observed.
> -   *
> -   * When the event is observed, the function will wait an extra tick before
> -   * returning.
>     *
>     * Note that in general, you should probably use createEventWaiter unless you

nit: comment refers to deleted/renamed function createEventWaiter

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8897255 [details]
Bug 1383622 (part 2) - update TPS to use the async bookmark API.

https://reviewboard.mozilla.org/r/168540/#review173800

As with the other patch, this is a much needed change that's great to see. I have a comple concerns in the bookmark.jsm code, but I don't think any of them matter if the tests pass. 

(For example, I vaguely recall that SetPosition has weird behavior for it's index, due to http://searchfox.org/mozilla-central/source/toolkit/components/places/nsINavBookmarksService.idl#450-452, but it's entirely possible update works the same way)

::: services/sync/tps/extensions/tps/resource/tps.jsm:1221
(Diff revision 1)
>    skipValidation() {
>      TPS.shouldValidateAddons = false;
>    }
>  };
>  
>  var Bookmarks = {

These need to be async now, and should await HandleBookmarks. You already have `await action[0].apply(this, action.slice(1));` (which is the caller) in RunNextTestAction so that should be it.

Comment 5

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

Comment on attachment 8897255 [details]
Bug 1383622 (part 2) - update TPS to use the async bookmark API.

I've no idea what went wrong, but that last patch was incomplete and didn't work at all. Here's a better one which also has a change that causes TPS to wait until the default browser bookmarks have been created, which was causing a race.

Comment 6

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

Comment on attachment 8897254 [details]
Bug 1383622 (part 1) - convert most of TPS to async/await.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168538/diff/1-2/

Comment 7

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

Comment on attachment 8897255 [details]
Bug 1383622 (part 2) - update TPS to use the async bookmark API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168540/diff/1-2/

Comment 8

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

Comment on attachment 8897255 [details]
Bug 1383622 (part 2) - update TPS to use the async bookmark API.

(I have no idea what I'm doing:)

Comment 9

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8897255 [details]
Bug 1383622 (part 2) - update TPS to use the async bookmark API.

https://reviewboard.mozilla.org/r/168540/#review174690

Still looks fine.

::: services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm:138
(Diff revisions 1 - 2)
> -        if (type == null || type == undefined || node.type == type)
> -          if (uri == undefined || uri == null || node.url.spec == uri.spec)
> -            return node.guid;
> +        let nodeType = this._typeMap.get(node.type);
> +        if (type == null || type == undefined || nodeType == type)
> +          if (uri == undefined || uri == null || node.uri.spec == uri.spec) {
> +            // Note that this is suspect as we return the *last* matching
> +            // child, which some tests rely on (ie, an early-return here causes
> +            // at least 1 test to fail). But that's a yak for another day.

Horrifying. It wouldn't surprise me if this were a bug though.

::: services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm:378
(Diff revisions 1 - 2)
>     * Updates the position of this places item within this item's current
>     * folder.  Use SetLocation to change folders.
>     *
>     * @param position The new index this item should be moved to; if null,
>     *        no changes are made; if -1, this item is moved to the bottom of
> -   *        the current folder
> +   *        the current folder. Otherwise, must be a string which is the

Ah, ok. That makes the original code make much more sense.

Comment 10

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

Comment on attachment 8897254 [details]
Bug 1383622 (part 1) - convert most of TPS to async/await.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168538/diff/2-3/

Comment 11

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

Comment on attachment 8897255 [details]
Bug 1383622 (part 2) - update TPS to use the async bookmark API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168540/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d229587489da
(part 1) - convert most of TPS to async/await. r=tcsc
https://hg.mozilla.org/integration/autoland/rev/60005e2d9ea5
(part 2) - update TPS to use the async bookmark API. r=tcsc

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This caused eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=123709378&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/ee42edbb6673f65ccd88cc2a06efa86bfab6e0c6

Comment 14

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

I really need to install that eslint outgoing hook :( Fix coming up

Comment 15

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

Comment on attachment 8897255 [details]
Bug 1383622 (part 2) - update TPS to use the async bookmark API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168540/diff/3-4/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/492d8960d85b
(part 1) - convert most of TPS to async/await. r=tcsc
https://hg.mozilla.org/integration/autoland/rev/aab3a105009e
(part 2) - update TPS to use the async bookmark API. r=tcsc

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/492d8960d85b
https://hg.mozilla.org/mozilla-central/rev/aab3a105009e