510984 - nsIAnnotationService IDL is lying about EXPIRE_NEVER

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Rob Campbell [:rc] (:robcee)]

from Jan Odvarko's comment in dev.platform...

To demonstrate the problem, I have created a simple extension, that
can be used to reproduce the problem.

Download here: http://www.janodvarko.cz/temp/testext.xpi

Here are steps to reproduce:

1) When the extension is loaded it tests two annotations. You should
see something like as follows the first time when it's loaded (in the
system console, using dump()):

Test; initialize
Test; annotation for: https://google.com/ -> false
Test; annotation for: https://getfirebug.com/ -> false

The false at the end is because the annotations don’t exist yet.

2) When you close Firefox window, you should see following in the
console:

Test; shutdown
Test; annotation for: https://google.com/ -> true
Test; annotation for: https://getfirebug.com/ -> true

Now there is true at the end, it’s because both annotations have been
created and tested when the extension is unloaded. Yes, now they
exist.

3) Restart your Firefox
Now I see following:

Test; initialize
Test; annotation for: https://google.com/ -> false
Test; annotation for: https://getfirebug.com/ -> true

The annotation for https://google.com/ is not there!

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

annotations on pages do expire with pages, could be that https://getfirebug.com/ had visits (or a bookmark), while https://google.com/ had none? in such a case closing firefox would expire pages without visits, removing the anno with the page.

Comment 2

[John J. Barton]

(In reply to comment #1)
> closing firefox would expire pages without visits,
> removing the anno with the page.

Is this true even for EXPIRE_NEVER? We mark URIs we never expect users to visit.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #2)
> (In reply to comment #1)
> > closing firefox would expire pages without visits,
> > removing the anno with the page.
> 
> Is this true even for EXPIRE_NEVER? We mark URIs we never expect users to
> visit.

yes, EXPIRE_NEVER for a page annotation means that the annotation won't be expired till the page exists in Places. Not doing this would mean the page won't never be expired, and that would be a privacy and performance problem.
The idl comments should be quite clear about that.

Comment 4

[John J. Barton]

(In reply to comment #3)
... 
> > Is this true even for EXPIRE_NEVER? We mark URIs we never expect users to
> > visit.
> 
> yes, EXPIRE_NEVER for a page annotation means that the annotation won't be
> expired till the page exists in Places. Not doing this would mean the page
> won't never be expired, and that would be a privacy and performance problem.
> The idl comments should be quite clear about that.

I don't understand what you are saying. ..."won't be expired till the page exists in Places". ?

In the documentation 
https://developer.mozilla.org/en/nsIAnnotationService
it says:

"If there is an important annotation that the user wants to keep, you should make sure that you use EXPIRE_NEVER. This will ensure the item is never completely deleted from the Places database."

Comment 5

[John J. Barton]

This bug is marked Mac OS X. Honza does it fail on Windows also?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #4)
> In the documentation 
> https://developer.mozilla.org/en/nsIAnnotationService
> it says:
> 
> "If there is an important annotation that the user wants to keep, you should
> make sure that you use EXPIRE_NEVER. This will ensure the item is never
> completely deleted from the Places database."

This doc is bogus, check the idl file please, i'll fix the doc.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

there is no way to put data in the database that persists beside user's choice, that would just be a privacy problem. The only way to persist an annotation is to create a bookmark for the page, that will stil expire if the user removes the bookmark.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to comment #5)
> This bug is marked Mac OS X. Honza does it fail on Windows also?
Yes

Honza

Comment 9

[Rob Campbell [:rc] (:robcee)]

changing platform flags.

Comment 10

[John J. Barton]

(In reply to comment #6)
> (In reply to comment #4)
> > In the documentation 
> > https://developer.mozilla.org/en/nsIAnnotationService
> > it says:
> > 
> > "If there is an important annotation that the user wants to keep, you should
> > make sure that you use EXPIRE_NEVER. This will ensure the item is never
> > completely deleted from the Places database."
> 
> This doc is bogus, check the idl file please, i'll fix the doc.

Exactly the same information is in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/public/nsIAnnotationService.idl

Comment 11

[John J. Barton]

I am nominating for blocking because we need a definitive answer. 
If this is a bug in nsIAnnotationService it could be affecting all Firefox uses via Places, causing poor usability without people realizing it (as is the case for Firebug). 

If this is not a bug in nsIAnnotationService but a bug in the documentation, then we will need to drop everything and create a fix for Firebug 1.4.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i'm not seeing anything different from what i said.

93     // For annotations that only live as long as the URI is in the database.
94     // A page annotation will expire if the page has no visits
95     // and is not bookmarked.
96     // An item annotation will expire when the item is deleted.
97     const unsigned short EXPIRE_NEVER = 4;

Comment 13

[John J. Barton]

I am also not seeing anything different from what I said:

132      * their privacy expectations about actions such as "Clear history." If
133      * there is an important annotation that the user wants to keep, you should
134      * make sure that you use EXPIRE_NEVER. This will ensure the item is never
135      * completely deleted from the Places database.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

hm i see that part of the idl was not fixed, the correct comment is the above one in comment 12, the other one in comment 13 is bogus. this bug should fix docs and idl comment imo.
Forcing pages to stay in the db if they are annotated, would just create problems, because nobody recalls to remove those annotations, especially add-ons.

Comment 15

[John J. Barton]

(In reply to comment #14)
..
> Forcing pages to stay in the db if they are annotated, would just create
> problems, because nobody recalls to remove those annotations, especially
> add-ons.

This reasoning makes no sense to me. It is the add-ons responsibility to remove annotations at the time appropriate. It should not be the responsibility of the service to decide that some annotation do not deserve to be stored. It's a database, not a policy service.

But that question is for a different day.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

first of all, add-ons can be disabled or disinstalled at any time, who will ensure that their annotations will be cleaned up?
second, you would annotate an uri, user clears history, but since you annotated that uri we just throw away all privacy expectations.
You know that expecting everyone doing wise things does not bring anything useful, just see bug 489173 for example.

The service has reponsibility to ensure correct datasource cleanup and privacy, a page that is never removed from its datasource does not respect neither of them.

If we want to store some sort of things like that, annotations should be linked to one-way hashes generated from uris, and be registered to a certain add-on id, the service should listen to add-ons uninstall and clean up those.

Comment 17

[John J. Barton]

(In reply to comment #16)
> first of all, add-ons can be disabled or disinstalled at any time, who will
> ensure that their annotations will be cleaned up?

Well that is a problem for the add-on manager to solve.

> second, you would annotate an uri, user clears history, but since you annotated
> that uri we just throw away all privacy expectations.

What privacy expectations? The URI added is not part of the history. 

> You know that expecting everyone doing wise things does not bring anything
> useful, just see bug 489173 for example.

I am only expecting the DB to keep the data we enter.

> 
> The service has reponsibility to ensure correct datasource cleanup and privacy,
> a page that is never removed from its datasource does not respect neither of
> them.

There is nothing about privacy involved here, the user has never visited the page and it  is not in the history.

> 
> If we want to store some sort of things like that, annotations should be linked
> to one-way hashes generated from uris, and be registered to a certain add-on
> id, the service should listen to add-ons uninstall and clean up those.

Huh? Anyway what addons will do is what Firebug did: create their own database because this one is unreliable. Then all of the good features you think you have created with the database are in fact thrown away in the full picture of the product.

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: idl comment patch v1.0

eventually i'd suggest to file an enh bug to get some really persisting annotation, but that respect privacy. i think annotating based on a one-way hash should be cool, we could just hash the uri and see if we have an anno for that hash. That clearly should have some sort of way to cleanup if the generating add-on is no more present in the profile.

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

you're right that we don't have a good way to store persistent informations on a page without bookmarking the page. but this does not mean we have to retain flaky implementations to allow that, if it's needed stuff, we should add a good implementation for it.

Comment 20

[Daniel Veditz [:dveditz]]

This is not blocking any release. We can consider approving a patch for branches if you get it reviewed, landed on trunk, and request approval.

Comment 21

[John J. Barton]

Ok, well I hope mak77 is correct then.

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 398156 [details] [diff] [review]
idl comment patch v1.0


>+     * NOTE: ALL PAGE ANNOTATIONS WILL GET DELETED WHEN THE PAGE IS REMOVED FROM
>+     * HISTORY IF THE PAGE IS NOT BOOKMARKED. This means that if you create an
>+     * annotation on an unvisited URI, it will get deleted when the browser
>+     * shuts down. Otherwise, things can exist in history as annotations but the

s/things/URIs/

>+     * user has no way of knowing it, potentially violating their privacy
>+     * expectations about actions such as "Clear history".
>+     * If there is an important annotation that the user wants to keep, you

"user or extension"

>+     * should add a bookmark for the page and use an EXPIRE_NEVER annotation.
>+     * This will ensure the annotation exists till the item is removed by the

s/till/until/

r=me w/ these changes.

john: i recommend you file an enhancement request for this use-case. it'd be better to have support for something like this, than for every extension to create their own database.

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/22bb0df90882

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

updated docs on https://developer.mozilla.org/en/nsIAnnotationService

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cd81ea145bdb

Comment 27

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv