820876 - Use bookmarks-observer category for the tagging service

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect, P2)

Description

[Marco Bonardo [:mak]]

There are a couple reasons:
1. the service listens to bookmarks changes to clean up tags, the implementer shouldn't rely on it being active since there's nothing enforcing that condition. if a bookmark is removed before the service is up, we don't properly cleanup tags
2. on startup it basically just adds the observer, this is the same as registering in the category

the only downside is that someone using Places but not tags will have this component inited, though on startup it does nothing, so it barely pays the price of an additional loaded service.

Comment 1

[Asaf Romano (gone)]

The cause is a typo in PlacesUtils:
    this.item.uri = PlacesUtils.bookmarks.getBookmarkURI(this.item.id);
...
      PlacesUtils.tagging.tagURI(this.new.URI, this.item.tags);

this.new.URI should be this.new.uri

Comment 2

[Asaf Romano (gone)]

oops, wrong bug.

Comment 3

[Marco Bonardo [:mak]]

the fix should be straight-forward, an xpcshell test is possible I guess. I'd only suggest to do a try run before pushing since some test may (wrongly) not expect tags notifications and it should be updated in such a case

Comment 4

[Marco Bonardo [:mak]]

The scope here is to modify the manifest where tagging service is defined, so that this service is registered in the bookmark-observers category, for example http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/toolkitplaces.manifest#20

Then we can remove all the code in function TaggingService() that so far is only registering it as an observer (it would be already) and registering a shutdown observer that is no more needed since the only thing it does is removing the observer.

Comment 5

[Ukwattage]

Hi,

I think this bug is still available so I would like to work on this.I have already built Firefox source code in windows but I'm new here could you guide me to start the things?

Comment 6

[Marco Bonardo [:mak]]

Hi sorry for late reply.
If you already have the build, you can start searching for the code to change through services like http://dxr.mozilla.org/ or http://searchfox.org/

Comment 4 contains the description of the code changes:
1. modify the manifest so that tagging service is registered in the bookmark-observers category (see how PlacesCategoriesStarter does that http://searchfox.org/mozilla-central/source/toolkit/components/places/toolkitplaces.manifest#20)
2. remove code from nsTaggingService that registers it as a bookmarks observer
3. remove code from nsTaggingService that registers it as a shutdown observer (and the observing code as well)

Comment 7

[Cryzek]

I'm trying this as my first bug.
As per your instructions, I edited the manifest and nsTaggingService.js files.
Could you guide me further

Comment 8

[Marco Bonardo [:mak]]

well, there isn't much more to do after those changes.
You should attach a patch, you can do either through moz-review (http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html) or just exporting a diff with a commit message and attaching it to the bug, adding a review flag on me.

Comment 9

[Cryzek]

Attachment: Fix to Bug 820876 - Use bookmarks-observer category for the tagging service [:mak]

Added tagging-service as bookmarks-observer in toolkitplaces.manifest.
Removed code that adds taggingService as a bookmarks-observer and shutdown-observer from nsTagggingService.js.

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

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8853063 [details]
Fix to Bug 820876 - Use bookmarks-observer category for the tagging service [:mak]

https://reviewboard.mozilla.org/r/125154/#review128442

it looks good, some code can still be removed, and let's see the results for the Try server build I just started.

::: toolkit/components/places/nsTaggingService.js:16
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Deprecated",
>                                    "resource://gre/modules/Deprecated.jsm");
>  
>  const TOPIC_SHUTDOWN = "places-shutdown";

This can now be removed, as well as the whole "observe" method

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8853063 [details]
Fix to Bug 820876 - Use bookmarks-observer category for the tagging service [:mak]

https://reviewboard.mozilla.org/r/125154/#review128444

::: toolkit/components/places/nsTaggingService.js:462
(Diff revision 1)
>    _xpcom_factory: XPCOMUtils.generateSingletonFactory(TaggingService),
>  
>    QueryInterface: XPCOMUtils.generateQI([
>      Ci.nsITaggingService
>    , Ci.nsINavBookmarkObserver
>    , Ci.nsIObserver

Oh, also this Ci.nsIObserver can be removed

Comment 12

[Marco Bonardo [:mak]]

There are some failures to handle.

TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js | remove_bookmark_tag_notification - [remove_bookmark_tag_notification : 500] [{"name":"onItemRemoved","arguments":[23,4,1,2,null,"t1XlwHyLFXtx","tags________",0]},{"name":"onItemRemoved","arguments":[24,23 deepEqual [{"name":"onItemRemoved","arguments":[24,23,0,1,"http://untag.example.com/","ZwZJ-fAEMQHw","t1XlwHyLFXtx",0]},{"name":"onItemCha
TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js | onItemChanged_tags_bookmark - [onItemChanged_tags_bookmark : 22] onItemRemoved(args[3]: itemType) - false == true

It may be an order of notifications problem, since now the tagging service registers earlier, but it's more plausible it's due to the tagging service removing a tag folder (and thus generating an additional onItemRemoved) when all the tagged uris for a given tag are removed.

You can run these tests using ./mach test path_to_test

Comment 13

[Marco Bonardo [:mak]]

to be clear, the tests should be fixed in this case.

Comment 14

[Leni Kadali [:leni1] [GMT+3]]

Attachment: Use bookmarks-observer category for the tagging service

Added tagging-service as bookmarks-observer in toolkitplaces.manifest.
Removed code that adds taggingService as a bookmarks-observer and shutdown-observer from nsTagggingService.js.
Removed the Components.utils.import code that imports Services.jsm, PlacesUtils.jsm, and Deprecated.jsm and
the constant referencing `places-shutdown`.
Removed the observe method for nsIObserver and its corresponding reference in QueryInterface: XPCOMUtils.generateQI()

Comment 15

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8882205 [details] [diff] [review]
Use bookmarks-observer category for the tagging service

(In reply to Marco Bonardo [::mak] from comment #13)
> to be clear, the tests should be fixed in this case.
So I'm supposed to work on the toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js file? This bit wasn't so clear to me.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Leni Kadali from comment #15)
> So I'm supposed to work on the
> toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
> file? This bit wasn't so clear to me.

yes. and test_nsINavBookmarkObserver.js.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 8882205 [details] [diff] [review]
Use bookmarks-observer category for the tagging service

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

::: toolkit/components/places/nsTaggingService.js
@@ -10,5 @@
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> -Components.utils.import("resource://gre/modules/Services.jsm");
> -Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "Deprecated",
> -                                  "resource://gre/modules/Deprecated.jsm");

why did you remove Services.jsm, PlacesUtils.jsm and Deprecated.jsm? They are still used, even after your removals.

PlacesUtils should just probably be changed to use defineLazyModuleGetter.

Comment 18

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Marco Bonardo [::mak] from comment #17)
> ::: toolkit/components/places/nsTaggingService.js
> @@ -10,5 @@
> >  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> > -Components.utils.import("resource://gre/modules/Services.jsm");
> > -Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
> > -XPCOMUtils.defineLazyModuleGetter(this, "Deprecated",
> > -                                  "resource://gre/modules/Deprecated.jsm");
> 
> why did you remove Services.jsm, PlacesUtils.jsm and Deprecated.jsm? They
> are still used, even after your removals.
Are they supposed to be used or removing the other mentions in this file is part of the bug as well?

Comment 19

[Marco Bonardo [:mak]]

(In reply to Leni Kadali from comment #18)
> > why did you remove Services.jsm, PlacesUtils.jsm and Deprecated.jsm? They
> > are still used, even after your removals.
> Are they supposed to be used or removing the other mentions in this file is
> part of the bug as well?

They are supposed to be used yet.

Comment 20

[Leni Kadali [:leni1] [GMT+3]]

Attachment: Use bookmarks-observer category for the tagging service

Added tagging-service as bookmarks-observer in toolkitplaces.manifest.
Removed code that adds taggingService as a bookmarks-observer and shutdown-observer from nsTagggingService.js.
Removed `const TOPIC_SHUTDOWN = "places-shutdown";` and the whole observe method.
Removed nsIObserver and its corresponding reference in QueryInterface: XPCOMUtils.generateQI()

Comment 21

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8885842 [details] [diff] [review]
Use bookmarks-observer category for the tagging service

I hope this is the correct version of the patch where I haven't deleted anything unnecessarily. If this is the case, then I will move to working on the tests `test_nsINavBookmarkObserver.js` and `test_bookmarks_notifications.js`.

Comment 22

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Marco Bonardo [::mak] from comment #17)
> PlacesUtils should just probably be changed to use defineLazyModuleGetter.
How would this be done? Something along the lines of `PlacesUtils.defineLazyModuleGetter()` like XPCOMUtils? If so, which module would need to be referenced?

Comment 23

[Marco Bonardo [:mak]]

(In reply to Leni Kadali from comment #22)
> (In reply to Marco Bonardo [::mak] from comment #17)
> > PlacesUtils should just probably be changed to use defineLazyModuleGetter.
> How would this be done? Something along the lines of
> `PlacesUtils.defineLazyModuleGetter()` like XPCOMUtils? If so, which module
> would need to be referenced?

XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                  "resource://gre/modules/PlacesUtils.jsm");

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 8885842 [details] [diff] [review]
Use bookmarks-observer category for the tagging service

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

This one looks OK, it would be nice to change PlacesUtils to be a lazy getter, but it's not critical.

Now, there are still the test failures in comment 12 to handle, likely by changing the order of the notifications. You can run the tests using ./mach test path_to_test

Comment 25

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Marco Bonardo [::mak] from comment #24)
> Now, there are still the test failures in comment 12 to handle, likely by
> changing the order of the notifications.
How exactly do I go about changing the order of notifications? I tried moving them around but the error persists. According to the last bit of the test output here: 

 0:05.42 TEST_STATUS: Thread-3 remove_bookmark_tag_notification FAIL [remove_bookmark_tag_notification : 550] [{"name":"onItemRemoved","arguments":[23,4,1,2,null,"UeJ8d_ss3J1O","tags________",0]},{"name":"onItemRemoved","arguments":[24,23 deepEqual [{"name":"onItemRemoved","arguments":[24,23,0,1,"http://untag.example.com/","xRjIRB6ZFYUz","UeJ8d_ss3J1O",0]},{"name":"onItemCha
/home/herabus/src/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js:expectNotifications/get/<:550
/home/herabus/src/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js:remove_bookmark_tag_notification:321
 0:05.42 LOG: Thread-3 INFO exiting test
 0:05.43 LOG: Thread-3 ERROR Unexpected exception 2147500036

It seems to me that the error lies within the `observer.check` method, not the order of notifications as I understand it, which would point to the cause being the tagging service removing a tag folder (and thus generating an additional onItemRemoved) when all the tagged uris for a given tag are removed as you said in comment 12

Comment 26

[Marco Bonardo [:mak]]

(In reply to Leni Kadali [:leni1] [GMT+3] from comment #25)
> It seems to me that the error lies within the `observer.check` method, not
> the order of notifications as I understand it, which would point to the
> cause being the tagging service removing a tag folder (and thus generating
> an additional onItemRemoved) when all the tagged uris for a given tag are
> removed as you said in comment 12

Mine was a guess, your suggestion is plausible if the test before was not initing the tagging service for some reason, otherwise in both cases we should have a notification about the folder going away when it contains no tags.
I'd suggest to print out in the check function the whole notifications array, compare how it was earlier and how it is now, and compare with the db contents through dump_table("moz_bookmarks");
Then you should be able to explain what changes before/after. If those changes make sense, you can just update the expected array to the new one.

Comment 27

[Marco Bonardo [:mak]]

Let's wontfix this, we should merge tags into bookmarks in the future, and the categories have been removed.