772211 - Sync should not use addVisit

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Attachment: Replacing use of addPage with async code.

Comment 1

[Gregory Szorc [:gps]]

Comment on attachment 640348 [details] [diff] [review]
Replacing use of addPage with async code.

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

The underlying API is synchronous. By changing it to synchronous, you introduce a race condition where the TPS test driver could advance before data has been added to the database.

The change to use the async APIs can be done: we just need to wait on the callback before returning from the function. You should be able to Cu.import("resource://services-common/async.js") and do something like:

  let cb = Async.makeSpinningCallback();
  
  PlacesUtils.asyncHistory.updatePlaces(places, {
    handleError: function Add_handleError() {
      cb(new Error("Error adding history entry."));
    }
    handleResult: function Add_handleResult() {
      cb(null);
    },
    handleCompletion: function Add_handleCompletion() {
      cb(null);
    }
  });
  cb.wait();

A proper solution would be to refactor the TPS APIs to be async. But, I don't think you want to tackle that ;)

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Replacing use of addPage with async code v2

Here we go. Local tests work, but with TryServer hiccups, I have not been able to get confirmation from TryServer.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gregory Szorc [:gps] from comment #1)
> Comment on attachment 640348 [details] [diff] [review]
> A proper solution would be to refactor the TPS APIs to be async. But, I
> don't think you want to tackle that ;)

Not quite yet :) By the way, is there a bug# for this? If you need OS.File features, feel free to ping me.

Comment 4

[Gregory Szorc [:gps]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Created attachment 642229 [details] [diff] [review]
> Replacing use of addPage with async code v2
> 
> Here we go. Local tests work, but with TryServer hiccups, I have not been
> able to get confirmation from TryServer.

TPS tests don't run on the buildbot infrastructure, so a Try push won't exercise this.

There is some documentation at https://developer.mozilla.org/en/TPS_Tests. Not sure if it is current.

> By the way, is there a bug# for this?

No. We generally don't want to touch TPS unless we have to.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Ah, I just understood that what I was patching is just a test. This patch is probably not very useful, then.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 642229 [details] [diff] [review]
Replacing use of addPage with async code v2

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

This looks good. But, I can't verify it because my m-c builds are doing:

 assertion failure: target, at /Users/gps/src/services-central/storage/src/mozStorageAsyncStatementExecution.cpp:190

If this lands in services-central, TPS will run on the server on checkin. So, we can always try to land. If it breaks, we back out.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 642229 [details] [diff] [review]
Replacing use of addPage with async code v2

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

There's something to fix in the callback (trying to interrupt twice), apart that I will be fine with gps final review pass, I don't have further comments.

::: services/sync/tps/extensions/tps/modules/history.jsm
@@ +107,5 @@
>      Logger.AssertTrue("visits" in item && "uri" in item,
>        "History entry in test file must have both 'visits' " +
>        "and 'uri' properties");
>      let uri = Services.io.newURI(item.uri, null, null);
> +    let places = {

ideally this is a single "place"

@@ +109,5 @@
>        "and 'uri' properties");
>      let uri = Services.io.newURI(item.uri, null, null);
> +    let places = {
> +      uri: uri,
> +      visits:[]

missing space after colon

@@ +116,5 @@
> +      places.visits.push({
> +        visitDate: usSinceEpoch + (visit.date * 60 * 60 * 1000 * 1000),
> +        transitionType: visit.type,
> +        referrerURI: null,
> +        sessionId: null

since this is a jsval, you don't have to define null properties, they are optional, so can be omitted.

@@ +122,3 @@
>      }
> +    if ("title" in item) {
> +      places.title = item.title;

I suppose (can be verified easily), you may just use title: item.title in the places object definition, if undefined/null the other side should get a void string and ignore it.

@@ +131,5 @@
> +        handleResult: function Add_handleResult() {
> +          cb(null);
> +        },
> +        handleCompletion: function Add_handleCompletion() {
> +          cb(null);

calling both in handleResult/Error and handleCompletion is going to notify and continue twice... you should just do one of those, I suggest using completion and track error/success internally.

Comment 8

[Marco Bonardo [:mak]]

David, do you plan to update the patch here, are you looking for help completing it?

Comment 9

[Marco Bonardo [:mak]]

I can find other addVisit entities in Sync:

services/sync/tps/extensions/tps/modules/history.jsm
services/sync/tests/unit/test_history_tracker.js

Comment 10

[Richard Newman [:rnewman]]

Bear in mind that Sync's history engine does handle visit-level granularity, so -- without yet taking the time to dig into the API; still drinking morning coffee! -- some use of visit-level APIs might be appropriate.

Comment 11

[Marco Bonardo [:mak]]

The fact is that nsINavHistoryService::AddVisit will not exist anymore, any visits update must go through mozIAsyncHistory::updatePlaces(), that is the async replacement for AddVisit. Nothing changes regarding granularity, it's just sync API being replaced by an async API.

Comment 12

[Richard Newman [:rnewman]]

(In reply to Marco Bonardo [:mak] from comment #11)
> The fact is that nsINavHistoryService::AddVisit will not exist anymore, any
> visits update must go through mozIAsyncHistory::updatePlaces(), that is the
> async replacement for AddVisit. Nothing changes regarding granularity, it's
> just sync API being replaced by an async API.

Got it. Putting this on the list of things to keep an eye on…

Comment 13

[David Teller [:Yoric] - still alive but not very active]

(In reply to Marco Bonardo [:mak] from comment #8)
> David, do you plan to update the patch here, are you looking for help
> completing it?

Sorry, your additional review had been lost in bugspam. I will try and update this soonish. How urgent is this patch?

Comment 14

[Marco Bonardo [:mak]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #13)
> Sorry, your additional review had been lost in bugspam. I will try and
> update this soonish. How urgent is this patch?

I'd like to remove addVisit in the Firefox 21 bracket, so by the end of Jan.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: Replacing use of addPage with async code v3

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: Replacing use of addPage with async code v4

Also applied to test_history_tracker.js

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Attachment: Replacing use of addPage with async code v5

Same one with yet another typo removed. Try: https://tbpl.mozilla.org/?tree=Try&rev=73ad8a53ec73

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 695083 [details] [diff] [review]
Replacing use of addPage with async code v5

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

Stealing gps's review flag, because he has other things to do!

Just nits and sadness :D

Thanks for taking care of this, David!

::: services/sync/tests/unit/test_history_tracker.js
@@ +35,5 @@
> +    uri: uri,
> +    visits: [ {
> +      visitDate: Date.now() * 1000,
> +      transitionType: 1
> +    } ]

Nit: surplus spaces, trailing commas. Should be:

  let place = {
    uri: uri,
    visits: [{
      transitionType: 1,
      visitDate: Date.now() * 1000,
    }],
  };

@@ +39,5 @@
> +    } ]
> +  };
> +  let cb = Async.makeSpinningCallback();
> +  PlacesUtils.asyncHistory.updatePlaces(place, {
> +    handleError: function Add_handleError() {

Nit: lose the "Add_".

@@ +47,5 @@
> +      cb(null);
> +    },
> +    handleCompletion: function Add_handleCompletion(status) {
> +       // Nothing to do
> +     }

Nit: correct indentation on these two lines, add period at end of comment, add trailing comma.

@@ +50,5 @@
> +       // Nothing to do
> +     }
> +   });
> +  // Spin the event loop to embed this async call in a sync API
> +  cb.wait();

Given that this is test code, I'd rather not, but meh! We'll fix it when we get rid of trackers.

::: services/sync/tps/extensions/tps/modules/history.jsm
@@ +120,5 @@
>      }
> +    if ("title" in item) {
> +      place.title = item.title;
> +    }
> +    let cb = Async.makeSpinningCallback();

Newline before "let cb", please.

@@ +130,5 @@
> +          cb(null);
> +        },
> +        handleCompletion: function Add_handleCompletion(status) {
> +          // Nothing to do
> +        }

Same comments apply here.

(It's a pity we have this code duplication. Oh well.)

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 695083 [details] [diff] [review]
Replacing use of addPage with async code v5

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

::: services/sync/tests/unit/test_history_tracker.js
@@ +34,5 @@
> +  let place = {
> +    uri: uri,
> +    visits: [ {
> +      visitDate: Date.now() * 1000,
> +      transitionType: 1

Please replace this hardcoded number with the proper PlacesUtils.history.TRANSITION_LINK

@@ +43,5 @@
> +    handleError: function Add_handleError() {
> +      cb(new Error("Error adding history entry"));
> +    },
> +    handleResult: function Add_handleResult() {
> +      cb(null);

I think there's no point in the explicit null argument, just omit it...

@@ +46,5 @@
> +    handleResult: function Add_handleResult() {
> +      cb(null);
> +    },
> +    handleCompletion: function Add_handleCompletion(status) {
> +       // Nothing to do

since you don't use status you can avoid defining it

::: services/sync/tps/extensions/tps/modules/history.jsm
@@ +120,3 @@
>      }
> +    if ("title" in item) {
> +      place.title = item.title;

much <3 for removing another instance of setPageTitle (bug 752217) :)

@@ +126,5 @@
> +        handleError: function Add_handleError() {
> +          cb(new Error("Error adding history entry"));
> +        },
> +        handleResult: function Add_handleResult() {
> +          cb(null);

ditto

@@ +129,5 @@
> +        handleResult: function Add_handleResult() {
> +          cb(null);
> +        },
> +        handleCompletion: function Add_handleCompletion(status) {
> +          // Nothing to do

ditto on status

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: Replacing use of addPage with async code v6

Try: https://tbpl.mozilla.org/?tree=Try&rev=79289967812d

Comment 21

[Richard Newman [:rnewman]]

Comment on attachment 699080 [details] [diff] [review]
Replacing use of addPage with async code v6

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

::: services/sync/tps/extensions/tps/modules/history.jsm
@@ +115,3 @@
>      for each (visit in item.visits) {
> +      place.visits.push({
> +        visitDate: usSinceEpoch + (visit.date * 60 * 60 * 1000 * 1000),

While you're here, could you lift that constant somewhere and make it meaningful: is it MICROSECONDS_PER_HOUR? Is that even correct?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9017046063ec

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9017046063ec