1362863 - bookmarks.onCreated should not be fired for bookmarks automatically created for the Library window

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[Sören Hentzschel]

Install a WebExtension with browser.bookmarks.onCreated.addListener. After opening the bookmarks library the onCreated event is fired 8x! It's reproducible in a new Firefox profile with Firefox Stable 52 and with Firefox Nightly 55.

STR:

1. You can use my add-on https://addons.mozilla.org/en-US/firefox/addon/bookmarks-organizer/ to test (it's not yet reviewed, so it's not yet signed; use version 1.0.3 because I will disable the code in the upcoming version 1.0.4 until this bug is fixed)
2. Start Firefox
3. click the browserAction button and have a look at the number of total bookmarks.
4. open the bookmarks library
5. repeat step 3

Actual result:

In Firefox Stable you can see a number of 7 bookmarks after step 3 and 15 bookmarks after step 5. In Firefox Nightly 55 there are 10 bookmarks after step 3 and 18 bookmarks after step 5.

Expected result:

No change of the total number of bookmarks.

In my add-on I use browser.bookmarks.onCreated.addListener and browser.bookmarks.onRemoved.addListener. I added a debug output in my local development version. Only the onCreated event is fired 8 times, the onRemoved event is not fired.

cc @bsilverberg who added these events.

Comment 1

[Sören Hentzschel]

Oh, after uploading 1.0.4 I will delete 1.0.3 from AMO because it's not yet reviewed. Here youou can find all old versions:
https://github.com/cadeyrn/bookmarks-organizer/tree/master/dist

Comment 2

[Bob Silverberg [:bsilverberg]]

Thanks for reporting the bug, Sören. The extra onCreated events are being fired for all of the bookmarks that are created automatically by Firefox for the left pane of the Library window when it is first opened. We should not be firing events for those bookmarks, but we do not currently have a way of knowing that those bookmarks should not be tracked.

Bug 1310295 has been created which should either fix the issue, or allow us to fix the issue, so I am going to update the title of this bug and set it up as blocked by bug 1310295.

Marco, do you agree with my assessment of this, or is there something we can currently do to exclude those events in our API code?

Comment 3

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #2)
> Bug 1310295 has been created which should either fix the issue, or allow us
> to fix the issue, so I am going to update the title of this bug and set it
> up as blocked by bug 1310295.
> 
> Marco, do you agree with my assessment of this, or is there something we can
> currently do to exclude those events in our API code?

As a workaround for now you could exclude these notifications by checking parentId in onItemAdded, if it's PlacesUIUtils.leftPaneFolderId or PlacesUIUtils.allBookmarksFolderId, then you can exclude them.

Obviously bug 1310295 is a better long term fix.

Comment 4

[Bob Silverberg [:bsilverberg]]

Thanks Marco. That sounds pretty reasonable for now. I'm not sure how urgent this is, and it's going to be pretty simple to fix, so I'm going to mark it as a good first bug for now.

Comment 5

[Bob Silverberg [:bsilverberg]]

Removing the priority and triaged tag for now so that an appropriate priority can be assigned during triage. It seems like a P3 to me, but others may disagree.

Comment 6

[Sören Hentzschel]

(In reply to Bob Silverberg [:bsilverberg] from comment #5)
> Removing the priority and triaged tag for now so that an appropriate
> priority can be assigned during triage. It seems like a P3 to me, but others
> may disagree.

I would say a broken API or part of a API should have a bit more priority because shipping nothing is almost always better than shipping something broken and this event is already part of Firefox 52. And if it's really pretty simple to fix then that's a reason more to do this as soon as possible. ;)

This API cannot really be used until this bug is fixed because it can give wrong results. And the bug is not mentioned in the documention, so other add-on developers don't know that this event is broken. They could use this event and release broken add-ons. Maybe there should be a warning in the documentation in the meantime. ;)

Comment 7

[Thomas Wisniewski]

I just tried the obvious fix, but it doesn't work. When I first open the bookmark panel (say with ctrl-b), PlacesUIUtils.(leftPaneFolderId|allBookmarksFolderId) are both undefined for the first 7 bookmarks sent to onItemAdded, and the eighth only has one of them defined. It looks like they're lazy getters, and I'm not sure how easy it would be to make a blocking getter for this purpose (or if we'd even want to do so).

Comment 8

[Sören Hentzschel]

Since the bug is still not fixed - is there a way for me as WebExtension developer to work around the issue?

Comment 9

[Bob Silverberg [:bsilverberg]]

It sounds like Thomas tried to fix this using the approach suggested by Marco, but was unsuccessful. I will take a look to see if I can figure out what needs to be done. As a stop-gap you could see if there's anything in the data returned for these bookmarks that would allow you to exclude them yourself. This is obviously not an ideal approach, but might allow you to proceed for now.

Comment 10

[Bob Silverberg [:bsilverberg]]

I see what Thomas means about PlacesUIUtils.leftPaneFolderId being lazy. It seems that it is created asynchronously the first time it is referenced, so just referencing it directly in onItemAdded doesn't work. If I create a const in ext-bookmarks.js that points to PlacesUIUtils.leftPaneFolderId, as well as one for PlacesUIUtils.allBookmarksFolderId, it seems to work, in that they are not undefined during onItemAdded, but I think that is also causing all of the virtual bookmarks to be created when I first call PlacesUIUtils.leftPaneFolderId, so that in itself seems to make the problem go away.

Marco, do you think defining consts for PlacesUIUtils.leftPaneFolderId and PlacesUIUtils.allBookmarksFolderId in ext-bookmarks.js is a valid thing to do, or is it bad that we're forcing the creation of the queries when they might not be needed? Can you suggest a different approach to the issue?

Oops, Marco isn't accepting needinfo requests as he's on PTO, so I'll flag him once he gets back from PTO.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #10)
> I see what Thomas means about PlacesUIUtils.leftPaneFolderId being lazy. It
> seems that it is created asynchronously the first time it is referenced

Those are lazy getters, but they are completely synchronous, when you access them, the folders are created.
The problem probably relies in the fact creating them may fire a further onItemAdded. So basically you enter a notifications loop, because to handle a notification you cause another one that you try to handle and so on... It's also likely to break Places.

You may have to do something like (pseudo-code):

if (this.ignoreNotification)
  return;
this.ignoreNotification = true;
try {
  // Check parentId against the left pane ids...
} finally {
  this.ignoreNotification = false;
}

> If I create a
> const in ext-bookmarks.js that points to PlacesUIUtils.leftPaneFolderId, as
> well as one for PlacesUIUtils.allBookmarksFolderId, it seems to work, in
> that they are not undefined during onItemAdded, but I think that is also
> causing all of the virtual bookmarks to be created when I first call
> PlacesUIUtils.leftPaneFolderId, so that in itself seems to make the problem
> go away.

This just seems to initialize all the left pane folder sooner, it may be fine provided it doesn't happen in the browser critical startup time (around the first paint), or it may cause unnecessary startup regressions.
Invoking even just once PlacesUIUtils.leftPaneFolderId should create all the bookmarks, but that also fires notifications, how do you plan to filter those? Is this supposed to happen before the first add-on is registered?

Comment 12

[Bob Silverberg [:bsilverberg]]

(In reply to Marco Bonardo [::mak] from comment #11)
> (In reply to Bob Silverberg [:bsilverberg] from comment #10)
> > I see what Thomas means about PlacesUIUtils.leftPaneFolderId being lazy. It
> > seems that it is created asynchronously the first time it is referenced
> 
> Those are lazy getters, but they are completely synchronous, when you access
> them, the folders are created.
> The problem probably relies in the fact creating them may fire a further
> onItemAdded. So basically you enter a notifications loop, because to handle
> a notification you cause another one that you try to handle and so on...
> It's also likely to break Places.
> 
> You may have to do something like (pseudo-code):
> 
> if (this.ignoreNotification)
>   return;
> this.ignoreNotification = true;
> try {
>   // Check parentId against the left pane ids...
> } finally {
>   this.ignoreNotification = false;
> }
> 

But even in this case, won't the value for PlacesUIUtils.leftPaneFolderId be undefined the first time I try to check against it? That would mean that I cannot accurately check against it the first time onItemAdded is fired for a bookmark.

> > If I create a
> > const in ext-bookmarks.js that points to PlacesUIUtils.leftPaneFolderId, as
> > well as one for PlacesUIUtils.allBookmarksFolderId, it seems to work, in
> > that they are not undefined during onItemAdded, but I think that is also
> > causing all of the virtual bookmarks to be created when I first call
> > PlacesUIUtils.leftPaneFolderId, so that in itself seems to make the problem
> > go away.
> 
> This just seems to initialize all the left pane folder sooner, it may be
> fine provided it doesn't happen in the browser critical startup time (around
> the first paint), or it may cause unnecessary startup regressions.
> Invoking even just once PlacesUIUtils.leftPaneFolderId should create all the
> bookmarks, but that also fires notifications, how do you plan to filter
> those? Is this supposed to happen before the first add-on is registered?

Perhaps those constants could be initialized inside the onItemAdded function, so they won't add to startup cost, and the cost will only be incurred the first time an item is actually added. I could then check each item added to make sure that neither its parent is one of the folders, and also make sure that *it* is not one of the folders. 

Does that sound like a reasonable solution?

Comment 13

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #12)
> But even in this case, won't the value for PlacesUIUtils.leftPaneFolderId be
> undefined the first time I try to check against it? That would mean that I
> cannot accurately check against it the first time onItemAdded is fired for a
> bookmark.

That's possible yes. If you get onItemAdded exactly for leftPaneFolderId, trying to get leftPaneFolderId would basically re-enter the getter. So you should at least executeSoon your notification handling... But then this would require delaying all of the notifications, since otherwise you could handle them in the wrong order.

> Perhaps those constants could be initialized inside the onItemAdded
> function, so they won't add to startup cost, and the cost will only be
> incurred the first time an item is actually added. I could then check each
> item added to make sure that neither its parent is one of the folders, and
> also make sure that *it* is not one of the folders.

As I said above, looks like there's potential for re-entering the creation code. You get a notification when leftPaneFolderId is added, you query leftPaneFolderId, but it's not set yet, so the code tries to set it again.

the only solutions I can think of so far are:
1. delay all the notifications handling, so you can use the leftPaneFolderId (maybe even check if it's still a getter so to not initialize it if unneeded). The downside is adding complication to your code.
2. Wait for bug 1310295 to make the Library left pane "virtual", so it won't create real bookmarks
3. Pay the library left pane init cost on Firefox startup. Likely a no-go for Quantum Flow.

Comment 14

[Bob Silverberg [:bsilverberg]]

What if I disable notifications, then initialize the values, then turn them back on, in the constructor of the observer, like so:

let observer = new class extends EventEmitter {
  constructor() {
    super();

    this.ignoreOnItemAdded = true;
    this.leftPaneRootId = PlacesUIUtils.leftPaneFolderId;
    this.allBookmarksRootId = PlacesUIUtils.allBookmarksFolderId;
    this.ignoreOnItemAdded = false;
  }

  onItemAdded(id, parentId, index, itemType, uri, title, dateAdded, guid, parentGuid, source) {
    if (this.ignoreOnItemAdded) {
      return;
    }

    do other stuff
  }
}

That will still incur the cost of left pane init, but it won't happen until ext-bookmarks.js is loaded, which I believe happens later in the cycle.

How does that sound?

Comment 15

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1362863 - Do not fire bookmarks.onCreated for bookmarks automatically created for the Library window

This ignores the onItemAdded event from nsINavBookmarkObserver for any bookmarks that
are automatically created for the Library window.

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

Comment 16

[Bob Silverberg [:bsilverberg]]

Rather than keep asking questions, I figured I'd just produce a patch and submit it for review. Please let me know where I've gone wrong.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8914452 [details]
Bug 1362863 - Do not fire bookmarks.onCreated for bookmarks automatically created for the Library window

https://reviewboard.mozilla.org/r/185784/#review190774

I would much rather see the core of this added to PlacesUIUtils to protect us (any consumer of the api) from any future changes, such as making these async or adding/removing any folders managed this way in PlacesUIUtils.  In onItemAdded we could just do something like

if PUIU.getVirtualFolderIDList().intersects([id, parentid]) return;

Comment 18

[Marco Bonardo [:mak]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> if PUIU.getVirtualFolderIDList().intersects([id, parentid]) return;

The problem is that PUIU knows about the new bookmark id only after the notification has fired, because the API returns AFTER notifying.
What we could do, crossing your ideas, is something like PUIU.isAddingLeftPaneItem() and ignore notifications if that returns true. Currently the PUIU creations are synchronous so there should be no big risk, and the only way to make those async would be to fix Bug 1310295, that would also remove this problem completely.
Would that work for you?

Comment 19

[Shane Caraveo (:mixedpuppy)]

Anything that gets us away from knowing PUIU internals is great by me, but if Bug 1310295 will happen soonish (maybe by 60?) and the patch for that can return and clean this side up, then I'm fine with accepting the patch here.

Comment 20

[Marco Bonardo [:mak]]

yes, that patch would remove the PUIU tricks like isAddingLeftPaneItem and as such it would also cleanup its consumers.

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8914452 [details]
Bug 1362863 - Do not fire bookmarks.onCreated for bookmarks automatically created for the Library window

https://reviewboard.mozilla.org/r/185784/#review191972

I'd like to know if my last idea could work for you first.

Comment 22

[Sören Hentzschel]

Bob said in #comment 2 ten months ago:

> Bug 1310295 has been created which should either fix the issue, or allow us to fix the issue, so I am going to update the title of this bug and set it up as blocked by bug 1310295.

Bug 1310295 has a patch now. Does the patch from bug 1310295 fix this issue? And is a fix for this issue still realistic for Firefox 60 (it's already late in the cycle)? I ask because Firefox 60 will be the next ESR release and it would be great if I could use the bookmarks.onCreated event without doing any version detection in my add-on if I want to support Firefox Mainstream and Firefox ESR at the same time once I bump the minimum required Firefox version to 60. A version detection is not the biggest problem, so I can do that if the issue will not be fixed in Firefox 60, but if it can be avoided it's, of course, better for me. ;-)

Comment 23

[Marco Bonardo [:mak]]

This has been fixed by bug 1310295. If the add-ons team thinks we should have some sort of hack for Firefox 60, we're open to suggestions.

Comment 24

[CosminB]

Attachment: Bug1362863.gif

I can  reproduce the issue on Firefox 59.0.2 (20180323154952) under Win 7 64-bit and Mac OS X 10.13.2 with the extension v1.0.3 from the comment1.

The number of the bookmarks it is "8" and after I open the bookmarks library it is "16".

This issue is verified as fixed on Firefox 61.0a1 (20180405104009) under Win 7 64-bit and Mac OS X 10.13.2.

Please see the attached video.