GNOME Bugzilla – Bug 780549
Crash when removing bookmark if URL has been modified
Last modified: 2017-08-24 08:26:24 UTC
After editing the URL of a bookmark and clicking the "remove" button, it still shows up in the popover. If the user tries to remove it again, the crash happens. To reproduce: 1. Open the "Bookmark Properties" window of bookmark. 2. Edit the URL. 3. Click "Remove bookmark". 4. Notice the bookmark has not been removed in the popover. 5. Repeat steps 1 and 3. (Open properties and remove) backtrace
+ Trace 237290
I can't reproduce this, but it seems that the crash is actually a failed assertion in ephy_bookmarks_manager_remove_bookmark(). I think it's not wise to crash when you don't find the bookmark to remove, especially when it's being searched by its URL (which can be edited). Supposing we don't want to get rid of the assertion, it would make more sense to search the bookmark by ID, because that cannot be edited. Otherwise, just return from the remove function if the bookmark is not found.
(In reply to Gabriel Ivașcu from comment #1) > I think it's not wise to crash when you don't find the bookmark to remove, > especially when it's being searched by its URL (which can be edited). I'm not sure. Trying to remove a bookmark that's not there is API misuse. We probably do want to crash in that case! I also was not able to reproduce the crash, but by following Eddy's steps I got some obviously-wrong behavior where the bookmark became duplicated instead. Also, the star in the location entry remained active, but it should have become inactive after the URL was edited to something else. This entire popover is really fragile I'm afraid; you can see I have dozens of commits to it trying to tighten up corner cases, but seems I missed a spot. :/
Apparently I missed a detail. The bookmark still shows up **only** in the tags section after the first removal. I omitted it without noticing, sorry. 4. Open the bookmarks popover, switch to the "tags" section and repeat removal.
(In reply to Eddy Castillo from comment #3) > Apparently I missed a detail. The bookmark still shows up **only** in the > tags section after the first removal. I omitted it without noticing, sorry. > > 4. Open the bookmarks popover, switch to the "tags" section and repeat > removal. Alright, I was now able to reproduce the bug. Apparently the problem lays in ephy-bookmarks-popover.c in create_bookmark_row(). This function creates a new EphyBookmarkRow object from a bookmark and stores a copy of the bookmark's URL and title into the row's table of associations with g_object_set_data_full(). The EphyBookmarkRow object is later attached to the tags list box. When a bookmark is deleted, the corresponding bookmark row is searched by the bookmark's URL in the tags list box in order to be deleted too. In your scenario, deleting a bookmark after editing its URL creates a mismatch between the actual bookmark's URL and the row's URL from the table of associations. Thus the EphyRowBookmark object is not found and not destroyed which also means that the bookmark object is not destroyed. Since the very same bookmark was previously removed from EphyBookmarksManager's sequence of bookmarks during the first delete, the second delete causes EphyBookmarksManager to delete a bookmark that doesn't exist from its point of view. This causes the failed assertion in the manager and therefore the crash. I'll be back with a patch that fixes this.
Created attachment 358269 [details] [review] bookmarks-popover: Don't snapshot URLs when creating new bookmark rows
Comment on attachment 358269 [details] [review] bookmarks-popover: Don't snapshot URLs when creating new bookmark rows For gnome-3-24 as well
Attachment 358269 [details] pushed as b6ced3e - bookmarks-popover: Don't snapshot URLs when creating new bookmark rows