1347386 - Removing Library left pane bookmarks breaks the Library window for the current session

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[Tim Nguyen :ntim]

Attachment: classic_bookmark_star-1.0.xpi

STR:
- Install attached extension
- Remove and re-add the same bookmark multiple times using the extension (click the star inside the URLbar)
- Open library window

AR:
- Library is blank

ER:
- Library should not be blank

Comment 1

[Tim Nguyen :ntim]

Sqlite.jsm:648

Error: Cannot remove a non-empty folder.
Stack trace:
transaction@resource://gre/modules/Bookmarks.jsm:1254:17
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
executeTransaction/promise</transactionPromise<@resource://gre/modules/Sqlite.jsm:593:28
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
executeTransaction/promise<@resource://gre/modules/Sqlite.jsm:563:32
promise callback*executeTransaction@resource://gre/modules/Sqlite.jsm:558:19
executeTransaction@resource://gre/modules/Sqlite.jsm:1389:12
removeBookmark/<@resource://gre/modules/Bookmarks.jsm:1250:11
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
executeBeforeShutdown@resource://gre/modules/Sqlite.jsm:374:25
executeBeforeShutdown@resource://gre/modules/Sqlite.jsm:1249:12
withConnectionWrapper/<@resource://gre/modules/PlacesUtils.jsm:1457:14
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
withConnectionWrapper@resource://gre/modules/PlacesUtils.jsm:1455:12
removeBookmark@resource://gre/modules/Bookmarks.jsm:1245:10
remove/<@resource://gre/modules/Bookmarks.jsm:439:20
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
TaskImpl_run@resource://gre/modules/Task.jsm:324:15
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
fetchBookmark/<@resource://gre/modules/Bookmarks.jsm:1129:22
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
executeBeforeShutdown@resource://gre/modules/Sqlite.jsm:374:25
executeBeforeShutdown@resource://gre/modules/Sqlite.jsm:1249:12
withConnectionWrapper/<@resource://gre/modules/PlacesUtils.jsm:1457:14
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
withConnectionWrapper@resource://gre/modules/PlacesUtils.jsm:1455:12
fetchBookmark@resource://gre/modules/Bookmarks.jsm:1126:10
remove/<@resource://gre/modules/Bookmarks.jsm:435:24

Comment 2

[Bob Silverberg [:bsilverberg]]

I will investigate this.

Comment 3

[Bob Silverberg [:bsilverberg]]

This is not a bug in the bookmarks API, it's a bug in the extension code. There are a number of errors in the extension code, but in this specific case, the code in `removeBookmark` in main.js is faulty which is causing the `matches` variable to contain an array of every bookmark and folder. When `matches` is iterated over, every bookmark and folder is removed, which results in this problem.

Comment 4

[Bob Silverberg [:bsilverberg]]

I am reopening this to investigate why deleting all the folders breaks the library.

Comment 5

[Bob Silverberg [:bsilverberg]]

The issue was that removing the reserved bookmarks folders was breaking the Library window. We really should not be allowing these folders to be removed via the bookmarks API.

Comment 6

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1347386 - Do not allow reserved bookmark folders to be removed via bookmarks API,

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

Comment 7

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8858912 [details]
Bug 1347386 - Do not allow reserved bookmark folders to be removed via bookmarks API,

This doesn't seem to address the issue that presented with the test extension that is attached to this bug, so more investigation is needed.

Comment 8

[Bob Silverberg [:bsilverberg]]

Marco, this bug originated with a WebExtension that was breaking the Library window by calling browser.bookmarks.remove(). I have done some investigation and have found that what seems to be breaking the Library window is the removal of some "hidden/reserved" bookmarks by the extension. Because of a flaw in the extension, it was sending a query to browser.bookmarks.search using {url: null}. That is currently legal (and works in Chrome), but it was resulting in an array of bookmarks being returned that includes these "hidden/reserved" bookmarks. I ran some tests with this locally, and you can see the results that are returned to me at [1]. 

The code in the extension then proceeded to remove all of the bookmarks found, which resulted in the removal of these "hidden/reserved" bookmarks, which then broke the Library window. I'm not sure what we need to do to ensure this doesn't happen in the future, but a couple of ideas are:

1. Do not return any of these special bookmarks from browser.bookmarks.search.
2. Prevent the removal of any of these special bookmarks.

I think #1 makes sense, although I'm not sure how to achieve that. The code that does the search is `queryBookmarks` in Bookmarks.jsm [2].

Even if we do implement #1, I think we should also implement #2, as it's theoretically possible for someone to still delete one of those special bookmarks using the id, even if it was not returned by bookmarks.search. Again, I'm not sure how to implement something that would prevent removal of those special bookmarks. I started with a patch that prevents removal of the known places root folders, but that didn't actually fix the problem. It seems like this is something that should perhaps be fixed in places, rather than in our API code, as we should probably not allow for removal of those folders in any situations.

I would very much appreciate it if you could take a look at this and let me know how you think we should proceed, and if you have any tips for implementing either #1 or #2 above, in case that is the route that we want to take.

Finally, for reference, when I say the Library window is "broken" what I mean is that choosing "Bookmarks" from the Firefox menu does not result in the window being opened. In the browser console, I see the messages:

NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.removeObserver]  browser-places.js:1479
	onBookmarksMenuHidden chrome://browser/content/browser-places.js:1479:9

TypeError: PlacesUtils.asContainer(...) is null[Learn More]  places.js:88:9
	PO_selectLeftPaneContainerByHierarchy chrome://browser/content/places/places.js:88:9
	PCH_showPlacesOrganizer chrome://browser/content/browser-places.js:698:7
	oncommand chrome://browser/content/browser.xul:1:1

[1] https://gist.github.com/bobsilverberg/fb7db706aebe447aa36df7935ac090bd
[2] http://searchfox.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1312

Comment 9

[Tim Nguyen :ntim]

> 1. Do not return any of these special bookmarks from browser.bookmarks.search.
I think it's a bad idea. 

It's better to set unmodifiable on the BookmarkTreeNode that is returned (though please make sure we can still modify the child bookmarks.

See https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks/BookmarkTreeNode#Type

Comment 10

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

Comment on attachment 8858912 [details]
Bug 1347386 - Do not allow reserved bookmark folders to be removed via bookmarks API,

https://reviewboard.mozilla.org/r/130896/#review134010

::: browser/components/extensions/ext-bookmarks.js:15
(Diff revision 1)
> +const NON_REMOVABLE_GUIDS = [
> +  PlacesUtils.bookmarks.rootGuid,
> +  PlacesUtils.bookmarks.menuGuid,
> +  PlacesUtils.bookmarks.toolbarGuid,
> +  PlacesUtils.bookmarks.unfiledGuid,
> +  PlacesUtils.bookmarks.mobileGuid,

these are already covered by
http://searchfox.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#622
So I'm not sure why this fix would make any difference? maybe the underlying code is ineffective?

I actually think the thing being removed by the add-on is part of the Library left pane folder, rather than this?
The left pane folder is a special root that is created by the frontend under rootGuid, it contains all the queries that make up the left pane folder of the Library (history, downloads, tags, All Bookmarks folder, and shortcuts to the root folders).
We cannot limit the API handling of these, since we use the same API to manage them.

To me looks like the bug is allowing browser.bookmarks.search return whatever thing, that is also one of the reasons I didn't like this API originally.

Also the tags filter in queryBookmarks looks wrong, indeed looks like it may return tag folders children, while it shouldn't (it checks parent, but not the grandparent).

I don't have an easy solution atm, the only thing that comes to my mind is to recursively query starting from the main roots (unfiled/menu/toolbar/mobile), that will likely be slower but will exclude any other root included tags and the left pane. This should use WITH RECURSIVE.

We cannot exclude just based on parent/grandparent because the leftpane root and its children are only known to the frontend, the backend doesn't know anything about those.
Longer term, the right solution is bug 1309930, that will remove any possibility to have roots unknown to the backend. Shorter term, it would be enough to fix bug 1310295, that would replace the Library left pane with a virtual query or an hardcoded UI.

Comment 11

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

(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> Finally, for reference, when I say the Library window is "broken" what I
> mean is that choosing "Bookmarks" from the Firefox menu does not result in
> the window being opened.

Does restarting firefox fix the problem?

Comment 12

[Bob Silverberg [:bsilverberg]]

(In reply to Marco Bonardo [::mak] from comment #10)
> Comment on attachment 8858912 [details]
> Bug 1347386 - Do not allow reserved bookmark folders to be removed via
> bookmarks API,
> 
> https://reviewboard.mozilla.org/r/130896/#review134010
> 
> ::: browser/components/extensions/ext-bookmarks.js:15
> (Diff revision 1)
> > +const NON_REMOVABLE_GUIDS = [
> > +  PlacesUtils.bookmarks.rootGuid,
> > +  PlacesUtils.bookmarks.menuGuid,
> > +  PlacesUtils.bookmarks.toolbarGuid,
> > +  PlacesUtils.bookmarks.unfiledGuid,
> > +  PlacesUtils.bookmarks.mobileGuid,
> 
> these are already covered by
> http://searchfox.org/mozilla-central/source/toolkit/components/places/
> Bookmarks.jsm#622
> So I'm not sure why this fix would make any difference? maybe the underlying
> code is ineffective?

Ah, thanks Marco. I didn't even look at that. I must have assumed the problem was the removal of those folders, but it likely was not, so the attached patch is definitely obsolete.

> 
> I actually think the thing being removed by the add-on is part of the
> Library left pane folder, rather than this?

Yes, I agree.

> The left pane folder is a special root that is created by the frontend under
> rootGuid, it contains all the queries that make up the left pane folder of
> the Library (history, downloads, tags, All Bookmarks folder, and shortcuts
> to the root folders).
> We cannot limit the API handling of these, since we use the same API to
> manage them.
> 
> To me looks like the bug is allowing browser.bookmarks.search return
> whatever thing, that is also one of the reasons I didn't like this API
> originally.
> 
> Also the tags filter in queryBookmarks looks wrong, indeed looks like it may
> return tag folders children, while it shouldn't (it checks parent, but not
> the grandparent).
> 
> I don't have an easy solution atm, the only thing that comes to my mind is
> to recursively query starting from the main roots
> (unfiled/menu/toolbar/mobile), that will likely be slower but will exclude
> any other root included tags and the left pane. This should use WITH
> RECURSIVE.
>

That does seem like a reasonable idea, although, as you say, would impact performance. This seems like a pretty extreme edge case, and restarting the browser does fix it, so maybe rather than making this change which would degrade performance we just leave it as is?
 
> We cannot exclude just based on parent/grandparent because the leftpane root
> and its children are only known to the frontend, the backend doesn't know
> anything about those.
> Longer term, the right solution is bug 1309930, that will remove any
> possibility to have roots unknown to the backend. Shorter term, it would be
> enough to fix bug 1310295, that would replace the Library left pane with a
> virtual query or an hardcoded UI.

That makes sense, and it looks like bug 1310295 is something that may be addressed soon. Is that likely? If so, should I just make that a blocker for this bug and then revisit this bug once 1310295 lands?

Kris, do you have any opinions about whether I should implement Marco's suggestion about querying specific folders above, or just wait for some other fixes to land regarding the left pane folders that will make fixing this easier and less of a performance hit?

Comment 13

[Bob Silverberg [:bsilverberg]]

(In reply to Marco Bonardo [::mak] from comment #11)
> (In reply to Bob Silverberg [:bsilverberg] from comment #8)
> > Finally, for reference, when I say the Library window is "broken" what I
> > mean is that choosing "Bookmarks" from the Firefox menu does not result in
> > the window being opened.
> 
> Does restarting firefox fix the problem?

As noted in comment #12, yes, restarting Firefox fixes the problem.

Comment 14

[Bob Silverberg [:bsilverberg]]

I am updating the summary of the bug once again. For a bit more detail on the summary:

The current bookmarks.search() API can return bookmarks that represent nodes of the left pane of the Library window. If an extension retrieves those, and then uses bookmarks.remove() to delete them, it breaks the Library window for the current session. Restarting Firefox does resolve the problem, but for the duration of the current session the Library window cannot be used and/or opened.

Comment 15

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

(In reply to Bob Silverberg [:bsilverberg] from comment #12)
> That does seem like a reasonable idea, although, as you say, would impact
> performance. This seems like a pretty extreme edge case, and restarting the
> browser does fix it, so maybe rather than making this change which would
> degrade performance we just leave it as is?

It sounds like a possibility considered this is an edge case that the browser can fix by itself on next startup. In any case an add-on should not remove stuff out of a query like that. I'm not even sure why it should remove bookmarks it didn't create itself, it's a good recipe for dataloss.

> > We cannot exclude just based on parent/grandparent because the leftpane root
> > and its children are only known to the frontend, the backend doesn't know
> > anything about those.
> > Longer term, the right solution is bug 1309930, that will remove any
> > possibility to have roots unknown to the backend. Shorter term, it would be
> > enough to fix bug 1310295, that would replace the Library left pane with a
> > virtual query or an hardcoded UI.
> 
> That makes sense, and it looks like bug 1310295 is something that may be
> addressed soon. Is that likely? If so, should I just make that a blocker for
> this bug and then revisit this bug once 1310295 lands?

I can't give an ETA, it's something both me and the Sync team wants (and now you), but everyone is extremely busy on other stuff: there's Quantum, Photon and other high priorities. I honestly don't know if that's feasible before 57 is out of the door.

Regardless what happens here, we should fix the tags filtering in queryBookmarks that is filtering out tag folders but not their contents afaict.

Comment 16

[Bob Silverberg [:bsilverberg]]

(In reply to Marco Bonardo [::mak] from comment #15)
> 
> Regardless what happens here, we should fix the tags filtering in
> queryBookmarks that is filtering out tag folders but not their contents
> afaict.

I opened bug 1358127 for that. Thanks Marco.

Comment 17

[Bob Silverberg [:bsilverberg]]

I've added a patch to bug 1358127 for the problems Marco identified with the current code. For now I'm just going to mark this as blocked by bug 1310295, and we can look at addressing the issue once that lands. I'm going to drop the priority as well.

Comment 18

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8858912 [details]
Bug 1347386 - Do not allow reserved bookmark folders to be removed via bookmarks API,

This is not what we need to do, so obsoleting this patch.

Comment 20

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

This should have been fixed by bug 1310295, it's no more possible to break the Library left pane through the bookmarks API

Comment 21

[marius.santa]

Attachment: library not empty.gif

Tested and reproduced issue in Firefox 55.0a1 (20170314030215).
Retested and verified in 61.0a1 (20180330100123) on Windows 10 64Bit/MacOS 10.13.3