Closed Bug 1481021 Opened Last year Closed Last year

Stop loading SpecialPowers into frame script globals

Categories

(Testing :: Mochitest, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Currently, SpecialPowers is loaded into most frame script globals. This has several side-effects, the most important of which is that it forces permissive COWs for those globals.

This is problematic enough as it is, since it allows all frame scripts to be able to pass objects to unprivileged content that they otherwise wouldn't. Which means that things that work under a test harness are likely to fail during actual use.

It's more problematic for the sake of landing bug 1480244, though, since that bug will be moving frame scripts to the shared JSM global, where permissive COWs are forbidden.

The obvious solution is to move the special powers API code into JSM globals, and only enable permissive COWs there. That would also allow us to make many aspects of the code significantly cleaner and easier to understand. Right now, SpecialPowers is loaded into a bunch of different kinds of scopes, and has a bunch of hacks to make it work in all of them. Ideally, it should just always be loaded as a JSM, and the SpecialPowers instances should be instantiated as appropriate.

The patches in this bug will only undertake the frame script aspect of this. Making the other loaders sane should probably be a follow-up, though.
Comment on attachment 8997678 [details]
Bug 1481021: Part 2 - Stop loading SpecialPowers into frame script scopes.

https://reviewboard.mozilla.org/r/261376/#review268508

mostly a rubber stamp here;  My understanding of specialpowers has declined over the years;  Looking through this code, I don't see any red flags.  Have you tested this set of changes on Android?
Attachment #8997678 - Flags: review?(jmaher) → review+
Comment on attachment 8997678 [details]
Bug 1481021: Part 2 - Stop loading SpecialPowers into frame script scopes.

https://reviewboard.mozilla.org/r/261376/#review268566

Did you forget to "hg add specialpowersFrameScript.js"?
(In reply to Andrew Swan [:aswan] from comment #4)
> Comment on attachment 8997678 [details]
> Bug 1481021: Part 2 - Stop loading SpecialPowers into frame script scopes.
> 
> https://reviewboard.mozilla.org/r/261376/#review268566
> 
> Did you forget to "hg add specialpowersFrameScript.js"?

Bah. Apparently yes. Which makes me wonder how I got a green try run...
Comment on attachment 8997677 [details]
Bug 1481021: Part 1 - Fix tests that rely on permissive COWs or SpecialPowers side-effects in frame script scopes.

https://reviewboard.mozilla.org/r/261374/#review268628

::: browser/components/newtab/test/browser/browser_topsites_section.js:68
(Diff revision 2)
>  // Check Topsites add
>  test_newtab({
>    // it should be able to click the topsites edit button to reveal the edit topsites modal and overlay.
>    test: async function topsites_add() {
>      let nativeInputValueSetter = Object.getOwnPropertyDescriptor(content.window.HTMLInputElement.prototype, "value").set;
> -    let event = new Event("input", {bubbles: true});
> +    let event = new content.Event("input", {bubbles: true});

Ouch.  Permissive COWs, eh?  :(

::: devtools/client/debugger/test/mochitest/code_frame-script.js:26
(Diff revision 2)
>  
>  this.call = function (name, args) {
>    dump("Calling function with name " + name + ".\n");
>  
>    dump("args " + JSON.stringify(args) + "\n");
> -  return XPCNativeWrapper.unwrap(content)[name].apply(undefined, args);
> +  return XPCNativeWrapper.unwrap(content)[name].apply(undefined, Cu.cloneInto(args, content));

I wonder whether we could get rid of XPCNativeWrapper altogether.... followup for that, perhaps.
Attachment #8997677 - Flags: review?(bzbarsky) → review+
Comment on attachment 8997678 [details]
Bug 1481021: Part 2 - Stop loading SpecialPowers into frame script scopes.

https://reviewboard.mozilla.org/r/261376/#review268630

::: testing/specialpowers/content/specialpowers.js:18
(Diff revision 2)
> +
> +var EXPORTED_SYMBOLS = ["SpecialPowers", "attachSpecialPowersToWindow"];
> +
> +ChromeUtils.import("resource://specialpowers/specialpowersAPI.js", this);
> +
> +Cu.forcePermissiveCOWs();

I don't see that being removed from specialpowersAPI.js.  Is that still coming?  Or do we need it here because we're no longer loading specialpowersAPI.js into this global or something?

::: testing/specialpowers/content/specialpowers.js:268
(Diff revision 2)
>  // In the case of Chrome mochitests that inject specialpowers.js as
>  // a regular content script
>  if (typeof window != "undefined") {
>    window.addMessageListener = function() {};
>    window.removeMessageListener = function() {};
> -  window.wrappedJSObject.SpecialPowers = new SpecialPowers(window);
> +  window.wrappedJSObject.SpecialPowers = new SpecialPowers(window, window);

Hmm.  Shouldn't the second arg be a message manager?  Why is passing "window" OK there?  I guess because we shimmed the add/removeMessageListener bits?

It might be good to document the setup with this fake message manager somewhere.

::: testing/specialpowers/content/specialpowersAPI.js:780
(Diff revision 2)
>             setTimeout(aCallback, 0);
>           // For mochitest-plain
>           else
> -           content.window.setTimeout(aCallback, 0);
> -       }
> -       delayAgain(delayAgain(callback));
> +           this.mm.content.setTimeout(aCallback, 0);
> +       };
> +       delayAgain(delayAgain.bind(this, callback));

This is presumably fixing a bug in that we were only doing one delay, not two?  But then why don't we just stop looking like we're delaying twice if delaying once works?
Attachment #8997678 - Flags: review?(bzbarsky) → review+
Comment on attachment 8997678 [details]
Bug 1481021: Part 2 - Stop loading SpecialPowers into frame script scopes.

https://reviewboard.mozilla.org/r/261376/#review268630

> I don't see that being removed from specialpowersAPI.js.  Is that still coming?  Or do we need it here because we're no longer loading specialpowersAPI.js into this global or something?

They both get loaded into separate JSM globals now, so they both need forcePermissiveCOWs. But neither of them get laoded into frame script globals anymore.

> Hmm.  Shouldn't the second arg be a message manager?  Why is passing "window" OK there?  I guess because we shimmed the add/removeMessageListener bits?
> 
> It might be good to document the setup with this fake message manager somewhere.

It *should*, but the lines above shim the window object to look enough like a message manager for most of its callers :( I was going to file a follow-up to get rid of those hacks.

> This is presumably fixing a bug in that we were only doing one delay, not two?  But then why don't we just stop looking like we're delaying twice if delaying once works?

Yeah. I filed a bug about that ages ago (bug 1464628). I don't know if delaying twice is actually necessary, but leaving that obviously broken code there when I was touching it anyway rankled...
(In reply to Kris Maglione [:kmag] from comment #5)
> (In reply to Andrew Swan [:aswan] from comment #4)
> > Comment on attachment 8997678 [details]
> > Bug 1481021: Part 2 - Stop loading SpecialPowers into frame script scopes.
> > 
> > https://reviewboard.mozilla.org/r/261376/#review268566
> > 
> > Did you forget to "hg add specialpowersFrameScript.js"?
> 
> Bah. Apparently yes. Which makes me wonder how I got a green try run...

Ah. Because I originally did this as a single commit, then did an `hg strip` and `hg crecord` to split it up, and the strip dropped the reference to the new file, so my second commit didn't pick it up.
Comment on attachment 8997677 [details]
Bug 1481021: Part 1 - Fix tests that rely on permissive COWs or SpecialPowers side-effects in frame script scopes.

https://reviewboard.mozilla.org/r/261374/#review268628

> Ouch.  Permissive COWs, eh?  :(

Yeah. I have more than half a mind to rip them out altogether at this point. :/ "Completely change a fundamental part of the way our security wrappers work because it makes testing slightly easier" just doesn't seem like a complelling argument, at this point...

> I wonder whether we could get rid of XPCNativeWrapper altogether.... followup for that, perhaps.

I think we probably can, at this point. ChromeUtils.waiveXrays/unwaiveXrays replace all of its original use cases, and now that legacy extensions are gone...

I'll file a follow-up.
> I have more than half a mind to rip them out altogether at this point.

That would be fantastic.  ;)
Comment on attachment 8997678 [details]
Bug 1481021: Part 2 - Stop loading SpecialPowers into frame script scopes.

https://reviewboard.mozilla.org/r/261376/#review268846
Attachment #8997678 - Flags: review?(aswan) → review+
Turns out some new code landed after my previous try run that actually runs in production builds but depends on the permissive CPOW behavior from the mochitest harness (and therefore works in tests but fails in the real world):

https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/browser/components/payments/content/paymentDialogFrameScript.js#92-104
Blocks: 1481640
https://hg.mozilla.org/integration/mozilla-inbound/rev/41bedc526dd6ec6b7e8c7be1c832ac60c81d6263
Bug 1481021: Part 1 - Fix tests that rely on permissive COWs or SpecialPowers side-effects in frame script scopes. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/c53c7b0249ad3359fbc9f144f2cf9ca3b6386c59
Bug 1481021: Part 2 - Stop loading SpecialPowers into frame script scopes. r=bz,jmaher,aswan
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e2128c54627239b50b70b0349d2fa93f88b4e2
Bug 1481021: Part 1 - Fix tests that rely on permissive COWs or SpecialPowers side-effects in frame script scopes. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecda4eb7431ab8d606c51d00fc6e34b25100904c
Bug 1481021: Part 2 - Stop loading SpecialPowers into frame script scopes. r=bz,jmaher,aswan
https://hg.mozilla.org/mozilla-central/rev/57e2128c5462
https://hg.mozilla.org/mozilla-central/rev/ecda4eb7431a
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/6e30e563d5f2fe2657f35ff0ec48af61749a2e96
chore(mc): Port Bug 1481021: Part 1 - Fix tests that rely on permissive COWs or SpecialPowers side-effects in frame script scopes. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/85247ce250af40e6eaf7abbfa2d99cf32a4b4302
Bug 1481021: Follow-up: Fix another test that relies on bad COW behavior. r=me,test-only
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.