I get a warning in terminal saying

(epiphany:21133): GLib-CRITICAL **: g_sequence_remove: assertion '!is_end (iter)' failed

That's actually when the post is already removed but still showing in the Tag and I try to remove it again.

Also, if I have two bookmarks in Misc, then after removing one of them and clicking the back arrow, the tag disappears from tag view, even though it should still be there because there is still one associated bookmark. And adding a new bookmark with the removed tag is not sufficient to bring it back.

...aaaaand it seems it's possible for the favorites tag to be disappear as well. Not sure how I got it into this state, but I'm not doing anything besides normal use.

It seems that Iulian is very careful in his popover code to handle these transitions properly, but spurious emissions of the EphyBookmark::tag-added signal are screwing it up. The signal is emitted several times (sometimes four times, sometimes twice) when I add a tag to a bookmark in the user interface. Merely clicking on the gear icon to open the bookmark details dialog causes the signal to be emitted twice. Investigating....

(In reply to Michael Catanzaro from comment #5)
> The signal is emitted several times (sometimes
> four times, sometimes twice)

Well not quite. Let's start with the simplest possible test:

 * jhbuild run epiphany -p
 * Navigate to example.com
 * Click the add bookmark icon
 * Create a new tag, Misc

This triggers a GAction activation that eventually ends in a call to ephy_bookmarks_properties_grid_actions_add_tag(). Here is an excerpt from that function:

  /* Create new tag with the given title */
  ephy_bookmarks_manager_create_tag (self->manager, text);

  /* Add tag to the bookmark's list of tags. */
  ephy_bookmark_add_tag (self->bookmark, text);

  /* Create a new widget for the new tag */
  widget = ephy_bookmark_properties_grid_create_tag_widget (self, text, TRUE);
  gtk_flow_box_insert (GTK_FLOW_BOX (self->tags_box), widget, -1);

First, ephy_bookmark_add_tag() adds the tag to the bookmark and emits the tag-added signal on the bookmark. It turns out there are three handlers connected to this signal, all bookmark_tag_added_cb() from ephy-bookmarks-popover.c. So the function executes three times in a row during signal emission. The signal is connected to for each EphyBookmarkRow (one in the All view, one in the Tags view, and one more in the Misc subview). But the signal handler is not written to expect this! It's written to be called just once.

Then, back in ephy_bookmarks_properties_grid_actions_add_tag(), the next line down is the call to ephy_bookmark_properties_grid_create_tag_widget(). That calls ephy_bookmark_properties_grid_tags_box_child_activated_cb() and that calls, yet again, ephy_bookmark_add_tag(), creating a new tag "Misc" with the same name as the previous tag (which should not have been allowed by the EphyBookmarksManager). And so tag-added gets emitted twice more (once for the EphyBookmarkRow in the All view, and once for the EphyBookmarkRow in the Misc subview; it doesn't exist in the Tags view anymore).

So there are two problems here:

 * The signal handler runs five times, when it's only expecting to run once.
 * The logic in ephy_bookmarks_manager_create_tag() that guards against adding a tag with the same name as another tag seems to be broken.

And we haven't even got anywhere near debugging what's going on when *removing* a tag, which is what this bug report is about after all, but I'm pretty sure it's a closely-related problem.

(In reply to Michael Catanzaro from comment #6)
> So there are two problems here:
> 
>  * The signal handler runs five times, when it's only expecting to run once.
>  * The logic in ephy_bookmarks_manager_create_tag() that guards against
> adding a tag with the same name as another tag seems to be broken.

Well three problems... we should not be creating two different tags in the first place, even if the EphyBookmarksManager would prevent it.

(In reply to Michael Catanzaro from comment #4)
> ...aaaaand it seems it's possible for the favorites tag to be disappear as
> well. Not sure how I got it into this state, but I'm not doing anything
> besides normal use.

I saw the favorites tag disappear as well. Restarting epiphany makes it appear again.

Also we have to keep the properties grid from the bookmark menu in sync with the bookmarks popover in the location entry when removing a tag; that's another good way to make it crash.

The following fixes have been pushed:
2a2a3a5 Bookmarks popover should only connect to tag changes once per bookmark
bb07e27 bookmark-properties-grid: Fix duplicate tag creation
3e4768e bookmark: include tag name in tag-added signal

Created attachment 343986 [details] [review]
Bookmarks popover should only connect to tag changes once per bookmark

Currently, the EphyBookmarksPopover connects to tag added/removed
signals on each EphyBookmark once for each EphyBookmarksRow that is
created for this bookmark. Usually that is exactly two, once for the All
view and once for the Tags view, but it can be three during an
intermediate state when adding a tag when the tag has just been added to
the Tags subview and not yet removed from the Tags main view. At any
rate, it should not be connected two or three times, it should be
connected once. So have EphyBookmarksManager emit these notifications
and get them from it instead, irrespective of the number of
EphyBookmarksRows that have been created.

Created attachment 343987 [details] [review]
bookmark-properties-grid: Fix duplicate tag creation

Here we're just trying to add a style class to a widget, but it
accidentally causes the new tag to be created a second time, leading to
serious breakage in the bookmarks popover.

Created attachment 343988 [details] [review]
bookmark: include tag name in tag-added signal

Even though it's not currently used, it's pretty strange to have a
tag-added signal that doesn't indicate which tag was added. This changes
the signal to parallel tag-removed.

(In reply to Michael Catanzaro from comment #9)
> Also we have to keep the properties grid from the bookmark menu in sync with
> the bookmarks popover in the location entry when removing a tag; that's
> another good way to make it crash.

Bug #777615

Hi. Are you sure this is fixed? I just removed a tag from a bookmark and it still showed till I collapsed the tag then clicked on it again.

Same as removing a bookmark completely.

(In reply to Hussam Al-Tayeb from comment #15)
> Hi. Are you sure this is fixed? I just removed a tag from a bookmark and it
> still showed till I collapsed the tag then clicked on it again.

I think I was deleting tags entirely instead of just removing them from the bookmark.

(In reply to Hussam Al-Tayeb from comment #15)
> Hi. Are you sure this is fixed? I just removed a tag from a bookmark and it
> still showed till I collapsed the tag then clicked on it again.

It seems to work reliably for me now. Are you able to reproduce the issue using my simple example.com test with no other bookmarks? That's what I'm doing to check; maybe it's still broken in a more-complicated case with more bookmarks?

With a more complex setup of at least two tags and around 5 bookmarks in each, it won't automatically update the view.
I noticed something similar as well.
When I open a bookmarked link, the favicon won't automatically show up in the tag view till I press back and then reopen the tag view.

I cleared ~/.config/epiphany ~/.local/share/epiphany/ and ~/.cache/epiphany.
Then I installed epiphany 3.22, imported firefox bookmarks from a html backup, and then installed epiphany master branch from git.
It imported my bookmarks again to bookbmarks.gvdb but still the same issue.
Sorry :/

Well let's start by getting a simpler reproducer. Could you attach a .gvdb file (bookmarks -> export bookmarks) that's sufficient to reproduce the broken state? Hopefully you can get it into a bad state using a temp profile (epiphany -p) by creating fake bookmarks and adding tags until you break it. Then export the bookmarks, open a new fresh profile, import them, and verify that you can reproduce the breakage. Then I can't use "works for me" as an excuse. ;)

Created attachment 344056 [details]
simple bookmarks.gvdb file

On a fresh profile with no imported bookmarks, it seems to update on removal but not re-adding.

Well the first thing I notice is the bookmarks import feature does not work at all... it says "bookmarks imported successfully!" but the popover continues showing the empty state view.

(In reply to Michael Catanzaro from comment #23)
> Well the first thing I notice is the bookmarks import feature does not work
> at all... it says "bookmarks imported successfully!" but the popover
> continues showing the empty state view.

It works but the view doesn't change.

Second thing I notice is that imported bookmarks don't get the usual signal connections, so the popover is going to be quite broken after a bookmarks import until Epiphany is closed and restarted.

(In reply to Hussam Al-Tayeb from comment #22)
> Created attachment 344056 [details]
> simple bookmarks.gvdb file
> 
> On a fresh profile with no imported bookmarks, it seems to update on removal
> but not re-adding.

I can reproduce; thanks for creating the test file.

OK, fixed... can you find another way to break it? ;)

The following fixes have been pushed:
cd930a9 bookmarks-popover: Restore bookmarks to tag view listbox when needed
4ab966e bookmarks-popover: Don't disconnect handlers when tag is removed

Created attachment 344087 [details] [review]
bookmarks-popover: Restore bookmarks to tag view listbox when needed

If a bookmark is removed from the tag view listbox while the listbox is
shown, then restored, it never comes back. Fix this.

Created attachment 344088 [details] [review]
bookmarks-popover: Don't disconnect handlers when tag is removed

We don't connect to anything in create_bookmark_row anymore so we don't
want to disconnect from anything here. Fortunately my mistake here was
harmless as nowhere except create_bookmark_row ever connected to
individual bookmarks.

The following fix has been pushed:
80bfba1 bookmarks-popover: Fix missing conditional in previous patch

Created attachment 344089 [details] [review]
bookmarks-popover: Fix missing conditional in previous patch

No reason to mess with the tag detail list box if it's not visible.

The following fix has been pushed:
06ecf14 bookmarks-popover: fix build error

Created attachment 344090 [details] [review]
bookmarks-popover: fix build error

I was too overconfident when copy/pasting.