561450 - (placesSessionID) Stop supporting session ids in Places

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Benjamin Stover (:stechz)]

Both History.cpp and nsNavHistory.cpp use the synchronous GetNewSessionId().  If session ID counter hasn't been initialized a sync SQL query is done to fetch the biggest ID, which is incremented by 1.

Comment 1

[Marco Bonardo [:mak]]

Hey Drew do you have ideas about this? since actually till we completely remove any sycnhronous addVisit api it's unlikely we can do this.

Comment 2

[Drew Willcoxon :adw]

The only thing I thought of, other than making GetNewSessionId callers async, is making history service initialization async.  The initial session ID counter could be cached then, along with whatever other I/O or blocking work that needs to happen on init, like, say, a database integrity check.  I haven't really checked all the history service clients to see if they'd be ruined by an async init.

Comment 3

[Marco Bonardo [:mak]]

Till we kill addVisit, addPageWithDetails and such calls adding synchronous visits, it's unlikely we can make anything more async in the initialization process.
That is bug 700250, Yoric took it but I don't think he plans to work on that anytime soon if you want to steal it, it likely requires to add an updatePlaces() wrapper in head_common.js that spins the async thread till the visit is added, to replace the thousands calls to addVisit in tests without having to rewrite them.
It also requires fixing the IE migrator (bug 591884), that on the other side may be broken from some time and should be ported to JS (mimicking the Chrome migrator structure in bug 505192) to improve its maintainability and to use the updatePlaces() JS API.

Comment 4

[Marco Bonardo [:mak]]

just a small updated here, what is left to do is basically removing addVisit() from some toolkit and browser tests, then we can remove the AddVisit() API, at that point this should be trivial to fix.

Comment 5

[Marco Bonardo [:mak]]

At this point, based on the "recent" changes to Places, I think the simplest thing is to kill session ids support.
Though, before doing that it's work to check if we need them for anything in-tree, and the use-cases in add-ons.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 8

[Marco Bonardo [:mak]]

To sum up the add-ons check we did yesterday, this affects about 7 add-ons, out of these 3 are outdated and no more supported, a couple are affected but will keep working properly, one is affected but has few more than 100 users.

Comment 9

[Marco Bonardo [:mak]]

ehr, a couple is actually 3, btw the point in the end is that this change may affect a few more than 100 users

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.1

just some cleanup of things I noticed in the previous version

Comment 11

[Asaf Romano (gone)]

Comment on attachment 728134 [details] [diff] [review]
patch v1.1

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

It's pretty astonishing how much code we have for a feature which was never really used.

::: toolkit/components/places/PlacesUtils.jsm
@@ +193,5 @@
> +      "https://bugzilla.mozilla.org/show_bug.cgi?id=561450");
> +    return this.nodeIsURI(aNode) && aNode.parent &&
> +           this.nodeIsQuery(aNode.parent) &&
> +           asQuery(aNode.parent).queryOptions.resultType ==
> +             Ci.nsINavHistoryQueryOptions.RESULTS_AS_VISIT;

What are the use cases for this method? If there are any use cases we may not want to deprecate it.

@@ -189,5 @@
>     *          A result node
>     * @returns true if the node is a URL item, false otherwise
>     */
> -  uriTypes: [Ci.nsINavHistoryResultNode.RESULT_TYPE_URI,
> -             Ci.nsINavHistoryResultNode.RESULT_TYPE_VISIT],

How many addons rely on this one? I would consider replacing it with a deprecated getter.

::: toolkit/components/places/nsINavHistoryService.idl
@@ +57,4 @@
>  
>    // Full visit nodes are deprecated and unsupported.
>    // This line exists just to avoid reusing the value:
>    // const unsigned long RESULT_TYPE_FULL_VISIT = 2;        // nsINavHistoryFullVisitResultNode

I still think it's odd to remove keep FULL_VISIT in place while removing VISIT :)

@@ +622,5 @@
>     * (which will comprise the majority of visit notifications) to save work.
>     *
>     * @param aVisitID        ID of the visit that was just created.
>     * @param aTime           Time of the visit
> +   * @param aSessionID      No longer supported, thus resolves to 0.

", thus resolves to 0" -> "(always set to 0)" would be better, I think.

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ -2490,5 @@
>        addition->mTransitionType = aTransitionType;
>        if (!history->EvaluateQueryForNode(mQueries, mOptions, addition))
>          return NS_OK; // don't need to include in our query
>  
> -      if (addition->IsVisit()) {

So, like with the js helper (nodeIsVisit), I wonder if it's worth keeping it around. Your call.

Comment 12

[Marco Bonardo [:mak]]

(In reply to Mano from comment #11)
> Comment on attachment 728134 [details] [diff] [review]
> patch v1.1
> 
> Review of attachment 728134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's pretty astonishing how much code we have for a feature which was never
> really used.
> 
> ::: toolkit/components/places/PlacesUtils.jsm
> @@ +193,5 @@
> > +      "https://bugzilla.mozilla.org/show_bug.cgi?id=561450");
> > +    return this.nodeIsURI(aNode) && aNode.parent &&
> > +           this.nodeIsQuery(aNode.parent) &&
> > +           asQuery(aNode.parent).queryOptions.resultType ==
> > +             Ci.nsINavHistoryQueryOptions.RESULTS_AS_VISIT;
> 
> What are the use cases for this method? If there are any use cases we may
> not want to deprecate it.

add-ons, more specifically an add-on with more than a thousands users.

> @@ -189,5 @@
> >     *          A result node
> >     * @returns true if the node is a URL item, false otherwise
> >     */
> > -  uriTypes: [Ci.nsINavHistoryResultNode.RESULT_TYPE_URI,
> > -             Ci.nsINavHistoryResultNode.RESULT_TYPE_VISIT],
> 
> How many addons rely on this one? I would consider replacing it with a
> deprecated getter.

Snowl, that is no more maintained, and a thunderbird add-on that copied all of our code included PlacesUIUtils for whatever reason
https://addons.mozilla.org/en-US/thunderbird/addon/wat-webapplicationtab/

> ::: toolkit/components/places/nsINavHistoryService.idl
> @@ +57,4 @@
> >  
> >    // Full visit nodes are deprecated and unsupported.
> >    // This line exists just to avoid reusing the value:
> >    // const unsigned long RESULT_TYPE_FULL_VISIT = 2;        // nsINavHistoryFullVisitResultNode
> 
> I still think it's odd to remove keep FULL_VISIT in place while removing
> VISIT :)

Didn't get this, we removed both...

 
> @@ +622,5 @@
> >     * (which will comprise the majority of visit notifications) to save work.
> >     *
> >     * @param aVisitID        ID of the visit that was just created.
> >     * @param aTime           Time of the visit
> > +   * @param aSessionID      No longer supported, thus resolves to 0.
> 
> ", thus resolves to 0" -> "(always set to 0)" would be better, I think.

lol, I changed it before attaching :)

> ::: toolkit/components/places/nsNavHistoryResult.cpp
> @@ -2490,5 @@
> >        addition->mTransitionType = aTransitionType;
> >        if (!history->EvaluateQueryForNode(mQueries, mOptions, addition))
> >          return NS_OK; // don't need to include in our query
> >  
> > -      if (addition->IsVisit()) {
> 
> So, like with the js helper (nodeIsVisit), I wonder if it's worth keeping it
> around. Your call.

nobody can call into this apart us, it's an internal helper.

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1.2

introduced a deprecated uriTypes getter, for now.

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69f4d3ba11a5

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/69f4d3ba11a5