Bug 561450 (placesSessionID)

Stop supporting session ids in Places

RESOLVED FIXED in mozilla22

Status

()

defect
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: stechz, Assigned: mak)

Tracking

(Blocks 1 bug)

unspecified
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [snappy:p1])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Depends on: 556400
Alias: placesSessionID

Updated

8 years ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
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.
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.
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.
Depends on: asyncHistory
Depends on: 724475
No longer depends on: 724475

Updated

7 years ago
Assignee: adw → nobody
Status: ASSIGNED → NEW
No longer depends on: asyncHistory
Depends on: 700250
Whiteboard: [snappy]

Updated

7 years ago
Whiteboard: [snappy] → [snappy:p1]
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.
No longer depends on: 700250
Depends on: 834457
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.
Summary: Make session ID generation asynchronous → Stop supporting session ids in Places
Duplicate of this bug: 821535
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Posted patch patch v1.0 (obsolete) — Splinter Review
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.
ehr, a couple is actually 3, btw the point in the end is that this change may affect a few more than 100 users
Posted patch patch v1.1 (obsolete) — Splinter Review
just some cleanup of things I noticed in the previous version
Attachment #728132 - Attachment is obsolete: true
Attachment #728134 - Flags: superreview?
Attachment #728134 - Flags: review?(mano)
Attachment #728134 - Flags: superreview? → superreview?(gavin.sharp)
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.
Attachment #728134 - Flags: review?(mano) → review+
(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.
Attachment #728134 - Flags: superreview?(gavin.sharp) → superreview+
Posted patch patch v1.2Splinter Review
introduced a deprecated uriTypes getter, for now.
Attachment #728134 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f4d3ba11a5
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/69f4d3ba11a5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 870581
No longer blocks: 870581
Depends on: 870581
Depends on: 1049747
You need to log in before you can comment on or make changes to this bug.