1448398 - Stop returning an unwrapped Components.interfaces from SpecialPowers

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

The drawback is that now if you try to do "foo.QueryInterface(SpecialPowers.Ci.nsIFoo)" where "foo" is not a SpecialPowers-wrapped object, that will fail.   That seems ok to me, though.  Consumers can either do "foo.QueryInterace(Ci.nsIFoo)" if they're in chrome or "SpecialPowers.wrap(foo).QueryInterface(SpecialPowers.Ci.nsIFoo)" if they're not.  In practice, the only non-chrome cases involve XBL, and I'm going to end up forcing those to use SpecialPowers.wrap() by making QueryInterface chromeonly (see bug 1448397).

The benefit is that this will allow me to fix bug 1389585 and bug 1389581.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Stop returning unwrapped Components.interfaces from SpecialPowers.Ci

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8961843 [details] [diff] [review]
Stop returning unwrapped Components.interfaces from SpecialPowers.Ci

Review of attachment 8961843 [details] [diff] [review]:
-----------------------------------------------------------------

s/Components.utils/Components.interfaces/ in attachment and bug summary please :) I was pretty confused until I read through the patch.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +237,5 @@
> +      // target.  We only do this when the target doesn't have its own
> +      // Symbol.hasInstance already.  Once we get rid of JS engine class
> +      // instance hooks (bug 1448218) and always use Symbol.hasInstance, we can
> +      // remove this bit (bug 1448400).
> +      return wrapIfUnwrapped(specialPowersHasInstance);

Nit: Can just be wrapPrivileged(specialPowersHasInstance)

@@ +278,5 @@
> +        // our target.  We only do this when the target doesn't have its own
> +        // Symbol.hasInstance already.  Once we get rid of JS engine class
> +        // instance hooks (bug 1448218) and always use Symbol.hasInstance, we
> +        // can remove this bit (bug 1448400).
> +        return { value: wrapIfUnwrapped(specialPowersHasInstance), writeable: true,

wrapPrivileged here, too.

::: toolkit/components/url-classifier/tests/mochitest/test_donottrack.html
@@ -14,5 @@
>  
>  <script class="testbody" type="text/javascript">
>  
> -var Cc = SpecialPowers.Cc;
> -var Ci = SpecialPowers.Ci;

I wonder if we should have an ESLint rule to stop people from doing things like this in chrome/browser tests...

Comment 3

[Boris Zbarsky [:bzbarsky]]

> s/Components.utils/Components.interfaces/ in attachment and bug summary please

Yikes, nice catch.

> Nit: Can just be wrapPrivileged(specialPowersHasInstance)

Will do.

> I wonder if we should have an ESLint rule to stop people from doing things like this in chrome/browser tests...

Might be nice.  On the other hand, it will fail in pretty obvious ways with this patch, so may not be worth it.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9945898d46e1
Stop returning unwrapped Components.interfaces from SpecialPowers.Ci.  r=kmag

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: followup to fix Windows-only test. pending

This is a chrome test, so it doesn't need to play SpecialPowers games.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/282b7f33195e
followup to fix Windows-only test.  r=kmag pending

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9945898d46e1
https://hg.mozilla.org/mozilla-central/rev/282b7f33195e