GNOME Bugzilla – Bug 632477
Fix crash in gnome-shell when loading extensions
Last modified: 2010-10-18 21:57:49 UTC
My XDG_DATA_DIR environment variable has some duplicate entries in it. I haven't looked into why, they are there, but they're making the shell crash for me. This is because it tries to load some extensions twice (once for each duplicated directory in my XDG_DATA_DIR path) and in the process runs into a reference counting bug. I'll attach two patches: one that fixes the reference counting bug, and another that prevents extensions from getting loaded twice in the face of duplicate paths in XDG_DATA_DIR.
Created attachment 172621 [details] [review] extensionSystem: Don't load extensions multiple times Right now if XDG_DATA_DIRS is set to have duplicate entries, then the extension system will try to load all extensions more than once. This commit prevents an extension from getting repeatedly loaded by checking if its uuid is already registered.
Created attachment 172622 [details] [review] st-theme: take extra reference on stylesheet st-theme stores some loaded stylesheets in both a hash table and a list. When adding the stylesheet to the hash table, it bumps the reference count of the stylesheet. When adding the stylesheet to the list, it does't change the reference count of the stylesheet. When removing a stylesheet from the hash table it properly unrefs it. However, when removing a stylesheet from the list it *also* unrefs it. This means all stylesheets get getting removed from the hash table are unref'd and free'd immediately and then later unref'd again by the list management code. Since the lifetime of the list is is very closely bound to the lifetime of the hashtable, one approach to fixing this problem would be to not unref the stylesheets in the list when removing them from the list. In this way, the stylesheets would be "owned" by the hash table. If we did this, we would need to make sure that we always remove stylesheets from the list everytime they're removed from the hash table (even when implicitly getting removed from an insert call that replaces it) The other approach is to make the list own its own reference to each stylesheet. This commit chooses the latter approach to minimize the amount of change needed to the code.
Review of attachment 172621 [details] [review]: Looks good.
Review of attachment 172622 [details] [review]: This one too, but the commit message is kind of too verbose IMO.
Created attachment 172640 [details] [review] st-theme: ref items in custom stylesheets list st-theme stores some loaded stylesheets in a custom stylesheets list. When removing items from this list it unrefs them, but when adding items to the list it neglects to ref them. This means that under certain circumstances the list will contain items that have already been freed.
Review of attachment 172640 [details] [review]: Sounds better i.e shorter ;)
Alright, will commit attachment 172640 [details] [review]. While rereading the commit message (when shortening it), it occurred to me we probably also need a more general solution for the duplicate style sheets problem than the fix mentioned in comment 1. I'll look into that.
I looked into problem alluded to in comment 8 a little off and on today... The st-theme code currently maintains a hash table of stylesheets keyed off of filename. That means if you import the same stylesheet uri multiple times only the last inserted stylesheet object will get freed when unload_stylesheet is called. Now some caveats: 1) I haven't hit this specific bug in the wild. I just noticed it when reading through the code and dealing with comment 5. 2) I believe that you would need a fairly pathological stylesheet layout to hit the bug. 3) we only call unload_stylesheet in error cases at the moment. Those caveats mean the right answer here is probably to just say "don't do that". The fix would involve making load_stylesheet return a structure containing the style sheet and a serial number, that's passed back to unload_stylesheet later, or something along those lines (which would make the api more awkward) I'm just going to forget about the issue and mark this bug FIXED.