Created attachment 161254 [details] [review]
Add a hack to mark functions deprecated

This is useful for keeping people from using methods that only fail in
certain circumstances, by making them fail in all circumstances
instead.

Review of attachment 161254 [details] [review]:

I think the basic idea here of trying to squash these bugs before they pop up again and again is great.

::: js/ui/environment.js
@@ +31,3 @@
 
+// deprecate @method such that any calls to it will fail
+function _deprecate(method, replacement, bug) {

This doesn't strike me as deprecation. Maybe _forbid()?

@@ +32,3 @@
+// deprecate @method such that any calls to it will fail
+function _deprecate(method, replacement, bug) {
+    let match = method.match(/(.*)\.([^.]*)/);

+ not * and I think it's clearer to anchor the regexp front and back (^ and $) - since you want to match the entire string, not some matching substring. (Though the particular regexp you've written will match all of any string with at least one period.)

@@ +46,3 @@
+        node = node[path[0]];
+        path.shift();
+    }

Hmm, it would be easier to read anyways to do:

 let node = eval('imports.gi.' + match[1] + '.prototype');

Marginally less efficient though.

@@ +48,3 @@
+    }
+
+    let msg = 'Calling deprecated function "' + method + '".';

Again, don't think deprecated is quite the right word

@@ +50,3 @@
+    let msg = 'Calling deprecated function "' + method + '".';
+    if (replacement)
+        msg = msg + ' You should use "' + replacement + '" instead.';

Might as well use += to mean 'append'.

@@ +52,3 @@
+        msg = msg + ' You should use "' + replacement + '" instead.';
+    if (bug)
+        msg = msg + ' See https://bugzilla.gnome.org/show_bug.cgi?id=' + bug;

Maybe just pass in a message rather than a bug number?

@@ +80,3 @@
+    // Shell.Global.prototype itself is read-only.
+    global.set_property_mutable('imports.gi.Shell.Global.prototype', 'set_property_mutable', true);
+    Shell.Global.prototype.set_property_mutable = undefined;

not convinced that this is actually necessary - hopefully our code review processes are sufficient to catch anybody using set_property_mutable() when they shouldn't, but sure...

::: src/shell-global.c
@@ +1330,3 @@
+
+void
+shell_global_set_property_mutable (ShellGlobal *global,

Basic docs would be good.

@@ +1349,3 @@
+      if (!JS_GetProperty (context, obj, parts[i], &val))
+        {
+          g_strfreev (parts);

At this point you've left an exception set in the JS context, but I don't think it will actually get thrown properly - I don't think GJS checks the context exception state after calling out to C. What I think I'd do is write a JSBool returning function to spidermonkey conventions, then a wrapper that uses gjs_log_exception() to log and clear the set exception if it fails.

You need to add val as a GC root before calling JS_GetProperty

(In reply to comment #2)
> +    global.set_property_mutable('imports.gi.Shell.Global.prototype',
> 'set_property_mutable', true);
> +    Shell.Global.prototype.set_property_mutable = undefined;
> 
> not convinced that this is actually necessary - hopefully our code review
> processes are sufficient to catch anybody using set_property_mutable() when
> they shouldn't, but sure...

I was worried about extensions abusing it, which seems inevitable if the functionality is there.

> At this point you've left an exception set in the JS context, but I don't think
> it will actually get thrown properly - I don't think GJS checks the context
> exception state after calling out to C.

I didn't re-test this code before filing the bug, but yes, ISTR that it does something not-exactly-right.

> What I think I'd do is write a JSBool
> returning function to spidermonkey conventions, then a wrapper that uses
> gjs_log_exception() to log and clear the set exception if it fails.

I'd wanted it to get thrown back at the JS code. I guess I could stick it into a GError.

> You need to add val as a GC root before calling JS_GetProperty

Really? Why? As I understand it, it should only be necessary to call JS_AddRoot on values that aren't reachable from some other root. And every value that val has throughout the loop is reachable from the global object.

(In reply to comment #3)

> > At this point you've left an exception set in the JS context, but I don't think
> > it will actually get thrown properly - I don't think GJS checks the context
> > exception state after calling out to C.
> 
> I didn't re-test this code before filing the bug, but yes, ISTR that it does
> something not-exactly-right.
> 
> > What I think I'd do is write a JSBool
> > returning function to spidermonkey conventions, then a wrapper that uses
> > gjs_log_exception() to log and clear the set exception if it fails.
> 
> I'd wanted it to get thrown back at the JS code. I guess I could stick it into
> a GError.

If getting an exception thrown is important, then, yes, I think you need to use a GError. even if it works at the moment, I think counting on JS exceptions propagating across the GI boundary is pretty dodgy.
 
> > You need to add val as a GC root before calling JS_GetProperty
> 
> Really? Why? As I understand it, it should only be necessary to call JS_AddRoot
> on values that aren't reachable from some other root. And every value that val
> has throughout the loop is reachable from the global object.

It's certainly not *in general* valid to assume that JS_GetProperty returns a value that is referenced elsewhere - if there is a property getter returned in C or in Javascript, it can return a newly created object. Now, in this case, yes, it seems like all the objects returned have to be rooted elsewhere, but I don't think we should rely on that type of analysis - we should just use JS_AddRoot before the loop to add 'val' as a root and make the code correct on its own.

Created attachment 161583 [details] [review]
Add a hack to block calls to certain introspected functions

This is useful for keeping people from using methods that only fail in
certain circumstances, by making them fail in all circumstances
instead.

Review of attachment 161583 [details] [review]:

Looks good to me.

::: src/shell-global.c
@@ +1376,3 @@
+  g_strfreev (parts);
+
+  if (!JS_GetPropertyAttributes (context, obj, property, &attrs, &found) || !found)

A little confusing if JS_GetPropertyAttributes doesn't set an exception if !found, but safe because gjs_log_exception() handles no exception set silently.

Attachment 161583 [details] pushed as fe542f8 - Add a hack to block calls to certain introspected functions