Commit f42e9d2833e1bf2edd59e50a1ec501bc1f8f3c33 in gobject-introspection changed the annotation for g_file_load_contents_finish from (out) (transfer full) to (out) (transfer full) (element-type guint8) (array length=length).
Since the annotation is indeed correct (g_file_load_contents_finish can return invalid UTF-8 or even NULL bytes), this requires GJS to support out arrays with explicit length.

Ugh =( This definitely sucks; I'm really wary of trying to hack in array+length combination into gjs this close to 3.0.

We could switch to using GLib.file_get_contents which we use elsewhere?

My suggestion for now is to add a shell wrapper that calls
g_file_load_contents_finish(), g_utf8_validates the result and returns it as a
string.

Created attachment 184819 [details] [review]
Add shell_get_file_contents_utf8_sync(), use it instead of gio temporarily

Adding correct annotations to Gio.File.load_contents revealed that gjs
doesn't actually support array+length combinations.  For 3.0 this would
be invasive to fix, so add a method to ShellGlobal which does what
we need.

Created attachment 184820 [details] [review]
Add shell_get_file_contents_utf8_sync(), use it instead of gio temporarily

Fix memory leak caught in self-review

Comment on attachment 184820 [details] [review]
Add shell_get_file_contents_utf8_sync(), use it instead of gio temporarily

Won't gjs throw an error itself if we use g_file_get_contents() and the return value can't be converted to UTF-8? So really, we don't need the shell function.

>     _addProvider: function(fileName) {
>         let file = Gio.file_new_for_path(global.datadir + '/search_providers/' + fileName);
...
>+        let path = file.get_path();

we don't need the GFile now. Just set path directly.

(In reply to comment #6)
> (From update of attachment 184820 [details] [review])
> Won't gjs throw an error itself if we use g_file_get_contents() 

No, we don't re-validate strings coming from C; char * means utf8.  Try it:
gjs> imports.gi.GLib.file_get_contents('/bin/true')
true,<invalid utf8 spewed to terminal here>,25144
gjs>

huh. ok, well, other than the not-needing-GFile-in-_addProvider, it all looks good then

Created attachment 184877 [details] [review]
Add shell_get_file_contents_utf8_sync(), use it instead of gio temporarily

Remove unneessary GFile

Note two things:

* The annotation for GLib.file_get_contents is also wrong, and we should probably convert the two users of it in the tree
* Fixing array+length in gjs really in my mind depends on a sane binary array story, which in turn depends on hard-requiring a new enough spidermonkey for Uint8Array.

Did you forget to squash fixes? Last patch looks the same as the previous one in terms of a GFile

(In reply to comment #11)
> Did you forget to squash fixes? Last patch looks the same as the previous one
> in terms of a GFile

I removed the GFile usage from searchProviders.js.  I didn't from extensionSystem.js, as it was used for more than just "path holder" there.

Review of attachment 184877 [details] [review]:

Generally looks correct and fixes a serious problem. A nit or two about error handling.

::: js/ui/search.js
@@ +280,3 @@
+            source = Shell.get_file_contents_utf8_sync(path);
+        } catch (e) {
+            return;

Silent skipping here is unfriendly, but if you just let it throw, it should get logged - it will cause subsequent search providers not to load, but I think that's probably OK. If not, you can add a surrounding try/catch/log on the whole function.

@@ +282,3 @@
+            return;
+        }
+        let [success, name, url, langs, icon_uri] = global.parse_search_provider(source);

Now that there isn't an extra layer of async here, this is going to cause all subsequent providers not to load if it throws.

Before closing this (but not for 3.0.0) we need to fix the other cases of g_file_get_contents() so that if the annotations are corrected on that we don't start having - though I'm not really sure I understand walter's example above - how did random garbage get through gjs_string_from_utf8()?

(In reply to comment #14)
>  though I'm not really sure I understand walter's example above -
> how did random garbage get through gjs_string_from_utf8()?

I think we were just hitting the first NUL character before any invalid UTF-8 in the /bin/true example actually...so I was wrong, sorry.  This fails readily for me:

$ dd if=/dev/urandom bs=128 count=1 | gjs -c "imports.gi.GLib.file_get_contents('/dev/stdin')"

Though actually, is g_utf8_to_utf16() defined not to throw an error (safely) on invalid UTF-8?  It looks like it tries, so maybe we're OK there.

(In reply to comment #15)
> (In reply to comment #14)
> >  though I'm not really sure I understand walter's example above -
> > how did random garbage get through gjs_string_from_utf8()?
> 
> I think we were just hitting the first NUL character before any invalid UTF-8
> in the /bin/true example actually...so I was wrong, sorry.  This fails readily
> for me:
> 
> $ dd if=/dev/urandom bs=128 count=1 | gjs -c
> "imports.gi.GLib.file_get_contents('/dev/stdin')"
> 
> Though actually, is g_utf8_to_utf16() defined not to throw an error (safely) on
> invalid UTF-8?  It looks like it tries, so maybe we're OK there.

It's a little subtle - g_utf8_to_utf16 handles stuff that doesn't properly meet the encoding, but it doesn't validate that the result is actually valid Unicode - it could include non-characters, unpared surrogates, etc. In general, I think is fine with handling random code-point strings, and when we convert back into UTF-8 we do validate. So, we'll get an exception but not until later than we might actually want.

But in any case, if we consider g_file_get_contents() to return a binary array, then we can't treat it as returning a UTF-8 string long-term.

(In reply to comment #13)
> Now that there isn't an extra layer of async here, this is going to cause all
> subsequent providers not to load if it throws.

which doesn't seem too bad for 3.0.0, since you shouldn't have invalid-UTF8-search-provider-files anyway

Created attachment 185105 [details] [review]
Fix to usage of shell_get_file_contents_utf8_sync()

Here's the minimal fixup to Colin's patch to make it not silently ignore
invalid-UTF-8. Tested to provide uniform behavior against:

 - Invalid UTF-8
 - File not readable for permissions
 - File with invalid XML

In all cases it throws with a descriptive backtrace and bails out of
loading the search providers.

Comment on attachment 185105 [details] [review]
Fix to usage of shell_get_file_contents_utf8_sync()

sure

Colin's patch and my fixup squashed together and pushed.

Attachment 184877 [details] pushed as 92f09a6 - Add shell_get_file_contents_utf8_sync(), use it instead of gio temporarily

Created attachment 185163 [details] [review]
extensionSystem: add missing import

extensionSystem can't load metadataFile content due missing import

*** Bug 646890 has been marked as a duplicate of this bug. ***

*** Bug 646889 has been marked as a duplicate of this bug. ***

Comment on attachment 185163 [details] [review]
extensionSystem: add missing import

oops

Review of attachment 185163 [details] [review]:

Pushed and released in gnome-shell-3.0.0.2.

*** Bug 647180 has been marked as a duplicate of this bug. ***

*** Bug 647187 has been marked as a duplicate of this bug. ***

Bug can be closed now, right?

Yep, thanks!

Is there another bug open for the problem in gjs?  The solution to this bug was just a workaround for that broader issue.

(In reply to comment #30)
> Is there another bug open for the problem in gjs?  The solution to this bug was
> just a workaround for that broader issue.

There are a few:
The oldest one from Johan:
https://bugzilla.gnome.org/show_bug.cgi?id=560567
An old one from Scott:
https://bugzilla.gnome.org/show_bug.cgi?id=582228
Newer ones from Giovanni:
https://bugzilla.gnome.org/show_bug.cgi?id=634253
https://bugzilla.gnome.org/show_bug.cgi?id=646632