GNOME Bugzilla – Bug 679099
"Automatic" GNOME Shell Extension updates
Last modified: 2012-07-10 18:37:17 UTC
Here's the giant patch stack. So, "automatic" right now means a DBus interface. I still have to work out the ChangeLog interface so users know what changed between versions, and I'm hoping to integrate that with the new "System Updates" stuff that hughsie is hacking on. When that lands, the flow will be changed to integrate with that; else, I will add a notification that pops up when extensions can be updated; pinging the site every 7 days or so on a cron job. I'm not entirely happy with a lot of the patches in here, especially with the callback/errback stuff in the error reporting system patch. I don't want to tie myself to a GDBus invocation, and since it's all async, I can't use regular exceptions. Maybe it's just time to add a Promise/Deferred system based on JS generators, like the perf stuff. I'd like to get any and all input on all of this, especially if it's by extension authors, shell developers, or John Stowers.
Created attachment 217575 [details] [review] shellDBus: Split extensions API out into a separate DBus interface The generic "Shell" interface was getting a bit too crowded.
Created attachment 217576 [details] [review] extensionDownloader: Fix loading of downloaded extensions We refactored the ExtensionSystem API to take an extension object, but forgot to update the downloader.
Created attachment 217577 [details] [review] browser-plugin: Prevent a copy when checking for valid extension UUIDs Instead of using g_strndup to copy the NPString that gets passed to us by the plugin host, just use the equally-as-good NPString directly. We still need to copy the string when passing it over DBus, as there's no easy way to construct a string GVariant from a length-prefixed string.
Created attachment 217578 [details] [review] browser-plugin: Rework scriptable method argument parsing and dispatch Since we're going to move to a much more complicated (async!) solution in a little bit, we're going to require a lot more machinery to handle that. To help with that, let's rework invocation dispatch so that it's more generic. Introduce a parse_args system similar to gjs_parse_args, use X Macros to help with the repetitive parts of the method dispatch. This shouldn't cause any API breaks, so API_VERSION should still be 4.
Created attachment 217579 [details] [review] extensionDownloader: Fix errors during error paths during installation
Created attachment 217580 [details] [review] extensionDownloader: Properly error out when downloading/parsing infos When the extension downloader was originally designed, the information downloading part was inserted at the last minute, along with the modal dialog as a security feature to make sure an extension didn't silently get installed on the user's machines either due to a security issue in the browser-plugin, or an XSS issue on the extensions website. Correct the mistake I made when writing the code; instead of dropping an error on the floor, log it correctly. This "bug" has already bitten a number of users who forgot to configure proxy settings in the control center.
Created attachment 217581 [details] [review] shellDBus: Add a real error reporting system to InstallExtensionRemote Instead of using the 'extension-state-changed' signal to relay errors, use DBus's native error mechanism to inform the method caller that the call has failed. This requires making the method actually asynchronous so that we don't block the browser, which is stuck waiting for a reply from the browser plugin. To ensure this, we need to modify the browser plugin API to ensure its extesion installation method is asynchronous. Additionally, this lets us remove the awful, broken hacks that we used when a user clicked the "Cancel" button, replacing it by a DBus return value.
Created attachment 217582 [details] [review] extensionSystem: Be saner at error handling Use our native JS error system in the "extension system" API, only using the signal/log-based error reporting at the last mile. Additionally, delete the directory if loading the extension failed, and report the error back over DBus.
Created attachment 217583 [details] [review] extensionDownloader: Clean up names of methods FromUUID is redundant.
Created attachment 217584 [details] [review] extensionUtils: Don't crash on startup for an empty directory
Created attachment 217585 [details] [review] extensionDownloader: Add update/blacklist support for extensions This is a bare-bones copy/replace. It does not implement ChangeLog support. If we cannot get System Updates integration, I will implement notification support.
Review of attachment 217575 [details] [review]: LGTM
Review of attachment 217576 [details] [review]: .
Review of attachment 217577 [details] [review]: I don't think it makes much difference, but ok.
Review of attachment 217578 [details] [review]: Nice cleanup in general. ::: browser-plugin/browser-plugin.c @@ +791,3 @@ +#undef METHOD + +static NPIdentifier list_extensions_id; While the double definition? And why no meta-code for properties and event handlers?
Review of attachment 217579 [details] [review]: Clearly so.
Review of attachment 217580 [details] [review]: .
Review of attachment 217581 [details] [review]: Makes a lot of sense.
Review of attachment 217582 [details] [review]: Generally ok. ::: js/ui/extensionSystem.js @@ +116,3 @@ } +function logExtensionError(uuid, error, state) { Unused parameter state?
Review of attachment 217583 [details] [review]: Ok
Review of attachment 217584 [details] [review]: This is kind of the opposite of the previous error handling patch. Should ExtensionFinder take an error callback instead? This way, it would logged in the usual place for extension errors, and thus reported to e.g.o owners. Also, using logError makes it look like a Shell error, while it is really an extension error.
Review of attachment 217585 [details] [review]: ::: js/misc/fileUtils.js @@ +47,3 @@ +} + +function recursivelyMoveDir(srcDir, destDir) { Why this is needed? Docs for g_file_move say that it automatically fallbacks to copy + delete in case rename() fails. ::: js/ui/extensionDownloader.js @@ +249,3 @@ + errback('LoadExtensionError', e); + return; + } This belongs to an earlier patch. ::: src/shell-util.c @@ +854,3 @@ + * A wrapper around g_mkdtemp() that actually works from + * introspection, by duplicating the input string. While + * we're at it, return a #GFile instead of a path string. You should fix g_mkdtemp instead of adding this. (It's just a matter of adding (transfer full) to both the parameter and the return value)
(In reply to comment #22) > Review of attachment 217585 [details] [review]: > > Why this is needed? Docs for g_file_move say that it automatically fallbacks to > copy + delete in case rename() fails. http://git.gnome.org/browse/glib/tree/gio/gfile.c#n2452 It will intentionally fail, saying it can't recurse. Maybe we should add the feature to Gio. I'll poke someone. > You should fix g_mkdtemp instead of adding this. > (It's just a matter of adding (transfer full) to both the parameter and the > return value) That's not true at all. Try it.
(In reply to comment #23) > (In reply to comment #22) > > Review of attachment 217585 [details] [review] [details]: > > > > Why this is needed? Docs for g_file_move say that it automatically fallbacks to > > copy + delete in case rename() fails. > > http://git.gnome.org/browse/glib/tree/gio/gfile.c#n2452 > > It will intentionally fail, saying it can't recurse. Maybe we should add the > feature to Gio. I'll poke someone. I'd say this belongs in Gio, yes. My primary concern anyway was to do atomic renames whenever possible (that is, when /tmp and /home live on the same partition) > > You should fix g_mkdtemp instead of adding this. > > (It's just a matter of adding (transfer full) to both the parameter and the > > return value) > > That's not true at all. Try it. Tried, and it works... Tried even valgrinding, no errors.
(In reply to comment #19) > Review of attachment 217582 [details] [review]: > > Generally ok. Did you mean to mark this ACN, then?
(In reply to comment #25) > (In reply to comment #19) > > Review of attachment 217582 [details] [review] [details]: > > > > Generally ok. > > Did you mean to mark this ACN, then? Yes, but with the comment addressed.
(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #19) > > > Review of attachment 217582 [details] [review] [details] [details]: > > > > > > Generally ok. > > > > Did you mean to mark this ACN, then? > > Yes, but with the comment addressed. Probably I clicked rejected instead of reviewed...
(In reply to comment #22) > ::: js/ui/extensionDownloader.js > @@ +249,3 @@ > + errback('LoadExtensionError', e); > + return; > + } > > This belongs to an earlier patch. I don't understand. Do you want me to split this section out in an earlier patch? (In reply to comment #21) > Review of attachment 217584 [details] [review]: > > This is kind of the opposite of the previous error handling patch. Should > ExtensionFinder take an error callback instead? This way, it would logged in > the usual place for extension errors, and thus reported to e.g.o owners. "Meh". We need a much better system for handling async errors. I'm really not the biggest fan of the errback/callback stuff I've already been doing.
(In reply to comment #24) > I'd say this belongs in Gio, yes. My primary concern anyway was to do atomic > renames whenever possible (that is, when /tmp and /home live on the same > partition) Not going to happen.
(In reply to comment #24) > > That's not true at all. Try it. > > Tried, and it works... Tried even valgrinding, no errors. Hm. It does work, but this seems like it's exploiting an implementation detail in gjs itself -- it still breaks with PyGObject.
Attachment 217575 [details] pushed as 7da0f39 - shellDBus: Split extensions API out into a separate DBus interface Attachment 217576 [details] pushed as 48b83f1 - extensionDownloader: Fix loading of downloaded extensions Attachment 217577 [details] pushed as 67689f1 - browser-plugin: Prevent a copy when checking for valid extension UUIDs Attachment 217578 [details] pushed as 6a117ac - browser-plugin: Rework scriptable method argument parsing and dispatch Attachment 217579 [details] pushed as 8915bb4 - extensionDownloader: Fix errors during error paths during installation Attachment 217580 [details] pushed as 1d1359b - extensionDownloader: Properly error out when downloading/parsing infos Attachment 217581 [details] pushed as 7949397 - shellDBus: Add a real error reporting system to InstallExtensionRemote Attachment 217583 [details] pushed as 23e86d7 - extensionDownloader: Clean up names of methods
(In reply to comment #28) > (In reply to comment #21) > > Review of attachment 217584 [details] [review] [details]: > > > > This is kind of the opposite of the previous error handling patch. Should > > ExtensionFinder take an error callback instead? This way, it would logged in > > the usual place for extension errors, and thus reported to e.g.o owners. > > "Meh". We need a much better system for handling async errors. I'm really not > the biggest fan of the errback/callback stuff I've already been doing. a-c-n, then. It's not public API anyway, we can fix it if we don't like it. (In reply to comment #29) > (In reply to comment #24) > > I'd say this belongs in Gio, yes. My primary concern anyway was to do atomic > > renames whenever possible (that is, when /tmp and /home live on the same > > partition) > > Not going to happen. Ok with the recursive JS fallback then. (In reply to comment #30) > (In reply to comment #24) > > > That's not true at all. Try it. > > > > Tried, and it works... Tried even valgrinding, no errors. > > Hm. It does work, but this seems like it's exploiting an implementation detail > in gjs itself -- it still breaks with PyGObject. An implementation detail we control, since we're also gjs developers (and really, there is no other way to it). Also, did you consider g_dir_make_tmp?
Created attachment 218207 [details] [review] extensionUtils: Don't crash on startup for an empty directory
Created attachment 218208 [details] [review] extensionDownloader: Move extension loading code to the install dialog Move the part that loads the extension to the callback. This makes the next patch a lot cleaner.
Created attachment 218209 [details] [review] extensionDownloader: Add update/blacklist support for extensions This is a bare-bones copy/replace. It does not implement ChangeLog support. If we cannot get System Updates integration, I will implement notification support. Thanks for the pointer to g_dir_make_tmp. We have a really schizophrenic platform.
Review of attachment 218207 [details] [review]: .
Review of attachment 218208 [details] [review]: Ok
Review of attachment 218209 [details] [review]: Ok
Attachment 218207 [details] pushed as 1363d30 - extensionUtils: Don't crash on startup for an empty directory Attachment 218208 [details] pushed as 539993b - extensionDownloader: Move extension loading code to the install dialog Attachment 218209 [details] pushed as 1e286e4 - extensionDownloader: Add update/blacklist support for extensions