Closed Bug 1448398 Opened 2 years ago Closed 2 years ago

Stop returning an unwrapped Components.interfaces from SpecialPowers

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Blocks: 1448400
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...
Attachment #8961843 - Flags: review?(kmaglione+bmo) → review+
> 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.
Summary: Stop returning an unwrapped Components.utils from SpecialPowers → Stop returning an unwrapped Components.interfaces from SpecialPowers
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9945898d46e1
Stop returning unwrapped Components.interfaces from SpecialPowers.Ci.  r=kmag
This is a chrome test, so it doesn't need to play SpecialPowers games.
Attachment #8962496 - Flags: review?(kmaglione+bmo)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/282b7f33195e
followup to fix Windows-only test.  r=kmag pending
Attachment #8962496 - Flags: review?(kmaglione+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/9945898d46e1
https://hg.mozilla.org/mozilla-central/rev/282b7f33195e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.