GNOME Bugzilla – Bug 665885
Allows defining circular categories --> crash
Last modified: 2015-09-29 15:43:28 UTC
How to reproduce: 1. add category A to category B 2. add category B to category A Result: gnome-documents: line 15: 3411 Segmentation fault (core dumped) /home/gnomedev/gnome-shell/install/bin/gjs-console -I /home/gnomedev/gnome-shell/install/share/gnome-documents/js -c "const Main = imports.main; Main.start();" "$@" version: git commit 8f78779dad3ffffacf9b2e6ad8d5dea8eada53a0 After that, every time you start gnome-documents it will hang in a loop that ends in the same crash.
Thanks for taking the time to report this bug. Without a stack trace from the crash it's very hard to determine what caused it. Can you get us a stack trace? Please see http://live.gnome.org/GettingTraces for more information on how to do so. Thanks in advance!
Program received signal SIGSEGV, Segmentation fault. general_composite_rect (imp=0x758050, op=PIXMAN_OP_OVER, src=0x863b400, mask=0x0, dest=0x8e3abc0, src_x=6, src_y=6, mask_x=12, mask_y=12, dest_x=12, dest_y=12, width=46, height=46) at pixman-general.c:116 116 { (gdb) bt
+ Trace 229251
It hangs in an infinite loop that makes it run out of memory.
Here is the start (missing in the previous one as it was too long):
+ Trace 229252
If you experience this crash, gnome-documents mail repeatedly crash on later restarts of the application. To allow the application to start, you'll need to refresh the tracker database and restart it. In a terminal: - tracker-control -r then - tracker-control -s It will take a few moments for your tracker database to repopulate.
I now fixed this in 0.3.91.
Well, with current git master you can still add collection A to B, and B to A, and get the crash. It seems the only thing that was fixed is that you can't add A to A?
(In reply to comment #7) > Well, with current git master you can still add collection A to B, and B to A, > and get the crash. If that is still the case, then we should not do that. If A is part of B, then B should not be allowed to be a part of A.
*** Bug 701087 has been marked as a duplicate of this bug. ***
In a recent discussion with the designers we agreed that we don't want nested collections. ie. collections inside collections. Doing that should address this bug.
In the current git master, the collections is broken. In recent if you select a document to add to collection, the dialog box doesn't show the collection list. Also if you add new collection, the checkbox doesn't work.
(In reply to Kunaal Jain from comment #11) > In the current git master, the collections is broken. In recent if you > select a document to add to collection, the dialog box doesn't show the > collection list. Also if you add new collection, the checkbox doesn't work. Yes, I have seen that happen. Most likely a fallout from bug 686461 The first thing to do here would be to disable the collections button if a collection has been selected. That won't require the dialog to work.
Created attachment 309980 [details] [review] selections: Prevent nested collections Disable the Collections button in the collections overview. If we are inside a collection, show the Collections button, but disable the delete entry for the active collection in the collections dialog.
Review of attachment 309980 [details] [review]: Looks generally good. I think here the only thing that might needs discussion is your patch makes it not possible to add collections to other collections in general; I'm not 100% sure that's what we want, as opposed to just not being able to add a collection to itself. ::: src/selections.js @@ +905,3 @@ + let windowMode = Application.modeController.getWindowMode(); + let activeCollection = Application.documentManager.getActiveCollection(); + if(windowMode == WindowMode.WindowMode.COLLECTIONS && !activeCollection) Missing space before paren
(In reply to Cosimo Cecchi from comment #14) > Review of attachment 309980 [details] [review] [review]: > > Looks generally good. I think here the only thing that might needs > discussion is your patch makes it not possible to add collections to other > collections in general; I'm not 100% sure that's what we want, as opposed to > just not being able to add a collection to itself. In my past conversations with Allan and Jakub they always said that we don't nested collections. CCed them.
Created attachment 310015 [details] [review] mainToolbar: Listen to changes when a collection is active
Created attachment 310016 [details] [review] selections: Prevent nested collections Disable the Collections button in the collections overview. If we are inside a collection, show the Collections button, but disable the delete entry for the active collection in the collections dialog.
Review of attachment 310015 [details] [review]: ::: src/mainToolbar.js @@ +113,3 @@ this._viewListButton = null; this._viewSettingsId = 0; + this._activeCollection = null; Should initialize this._infoUpdatedId = 0 here. @@ +251,3 @@ + if (activeCollection) + this._infoUpdatedId = activeCollection.connect('info-updated', Lang.bind(this, this._setToolbarTitle)); + else Should check for this._infoUpdatedId != 0 here.
Review of attachment 310016 [details] [review]: Looks good to me provided that we want to prevent nested collections completely.
Created attachment 310076 [details] [review] mainToolbar: Listen to changes when a collection is active
Two thumbs up for avoiding nesting.
Comment on attachment 310016 [details] [review] selections: Prevent nested collections Pushed to master.
Review of attachment 310076 [details] [review]: There is one issue. If we open a collection, then preview an item and come back before opening the collection dialog, then the toolbar is not updated on rename. The reason is that we destroy and recreate the toolbars on window-mode changes. So the new OverviewToolbar does't have the signal connection set up when we return from the preview. ::: src/mainToolbar.js @@ +252,3 @@ + if (activeCollection) + this._infoUpdatedId = activeCollection.connect('info-updated', Lang.bind(this, this._setToolbarTitle)); + else The existing style is to use the braces. @@ +256,3 @@ + this._activeCollection.disconnect(this._infoUpdatedId); + this._infoUpdatedId = 0; + } I think we need to disconnect on 'destroy' because activeCollection can outlive the toolbar.
Created attachment 310177 [details] [review] mainToolbar: Listen to changes when a collection is active
Review of attachment 310177 [details] [review]: ::: src/mainToolbar.js @@ +139,3 @@ this.connect('destroy', Lang.bind(this, function() { + if (this._infoUpdatedId == 0) This should check for != 0
Created attachment 310215 [details] [review] mainToolbar: Listen to changes when a collection is active
Review of attachment 310215 [details] [review]: Looks good to me.
Created attachment 312182 [details] [review] mainToolbar: Fix typo in the variable name Fallout from cb2039117e6ea422a810885d0622131e02f93f8e
Review of attachment 312182 [details] [review]: Thanks for catching this!