911573 - Remove UniversalXPConnect exemption from the access check for Components

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: Mochitest fixup

I'm astonished so many tests still depend on Components-in-content.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Enable and use specialpowers in jsreftest

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Stablize the pointerlock test

On OS X, mousemove events are somehow fired twice before the pointer lock is granted.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: The main patch

Try results:
https://tbpl.mozilla.org/?tree=Try&rev=d4b2a9b609df
https://tbpl.mozilla.org/?tree=Try&rev=8b0eb89d2108

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 798293 [details] [diff] [review]
Enable and use specialpowers in jsreftest

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

Thank you for doing this, it looks like a nice improvement! But, I have no idea at all how this should work, or even who would know how it should work, so I'm going to punt on reviewing the semantics of the change. It's browser security related, so I'm guessing Bobby will at least know who to send this to.

For what it's worth, I can't see any non-semantics issues with it, so r=me on that.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 798292 [details] [diff] [review]
Mochitest fixup

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

Holy Cow! This is incredible. As someone who's spent countless hours removing this crap from our test suite, I'm extremely impressed and appreciative. It must have been a ton of work.

::: content/base/test/test_bug424359-1.html
@@ +49,5 @@
>              message);
>  }
>  
>  function testHtmlSerializer_1 () {
> +  const de = SpecialPowers.Ci.nsIDocumentEncoder

Needs a |;|, though to be fair the issue was pre-existing.

::: content/base/test/test_bug424359-2.html
@@ +49,5 @@
>              message);
>  }
>  
>  function testHtmlSerializer_1 () {
> +  const de = SpecialPowers.Ci.nsIDocumentEncoder

same.

::: content/base/test/test_bug498433.html
@@ +48,5 @@
>              message);
>  }
>  
>  function testHtmlSerializer_1 () {
> +  const de = SpecialPowers.Ci.nsIDocumentEncoder

same

::: content/base/test/test_bug541937.html
@@ +20,5 @@
>  <pre id="test">
>  <script class="testbody" type="text/javascript">
>  
>  function testSerializer () {
> +  const de = SpecialPowers.Ci.nsIDocumentEncoder

same

::: toolkit/components/passwordmgr/test/test_basic_form_observer_autocomplete.html
@@ +91,5 @@
>    is($_(2, "uname").value, "testuser", "Checking for filled username 2");
>    is($_(2, "pword").value, "testpass", "Checking for filled password 2");
>  
>    // Remove the observer
> +  //os.removeObserver(TestObserver, "passwordmgr-found-form");

Hm, why do we stop removing the observer here? Won't that cause us to leak?

::: toolkit/components/passwordmgr/test/test_basic_form_observer_autofillForms.html
@@ +95,5 @@
>    SpecialPowers.setBoolPref("signon.autofillForms", true);
>  
>    // Remove the observer
> +  //os.removeObserver(TestObserver, "passwordmgr-found-form");
> +  //os.removeObserver(TestObserver, "passwordmgr-found-logins");

same here.

::: toolkit/components/passwordmgr/test/test_basic_form_observer_foundLogins.html
@@ +174,5 @@
>    // was set to false (didntFillReason == noAutofillForms) is done in
>    // test_basic_form_observer_autofillForms.html.
>  
>    // Remove the observer
> +  //os.removeObserver(TestObserver, "passwordmgr-found-logins");

and here.

::: toolkit/components/prompts/test/test_bug620145.html
@@ +76,5 @@
>    e = document.createEvent("MouseEvent");
>    e.initEvent(type, false, false, win, 0, 1, 1, 1, 1,
>                false, false, false, false, 0, null);
>    var utils = SpecialPowers.getDOMWindowUtils(win);
> +  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

Hm, why do we need this?

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 798295 [details] [diff] [review]
The main patch

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

\o/

Comment 7

[Bobby Holley (:bholley)]

How many calls to enablePrivilege does the first patch remove, btw? I'm curious how far this takes us towards eliminating it entirely.

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to Bobby Holley (:bholley) from comment #5)
> :::
> toolkit/components/passwordmgr/test/test_basic_form_observer_autocomplete.
> html
> @@ +91,5 @@
> >    is($_(2, "uname").value, "testuser", "Checking for filled username 2");
> >    is($_(2, "pword").value, "testpass", "Checking for filled password 2");
> >  
> >    // Remove the observer
> > +  //os.removeObserver(TestObserver, "passwordmgr-found-form");
> 
> Hm, why do we stop removing the observer here? Won't that cause us to leak?

removeObserver threw a cryptic error when I replaced enablePrivilege with SpecialPowers.

> ::: toolkit/components/prompts/test/test_bug620145.html
> @@ +76,5 @@
> >    e = document.createEvent("MouseEvent");
> >    e.initEvent(type, false, false, win, 0, 1, 1, 1, 1,
> >                false, false, false, false, 0, null);
> >    var utils = SpecialPowers.getDOMWindowUtils(win);
> > +  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> 
> Hm, why do we need this?

The test couldn't press the button in the alert without adding this. (Looks like the timeout handler was not executed at all during the alert.)

I'll remove passwordmgr and prompt changes for now, beause I can't land the final patch anyway until Talos is fixed. It would be meaningful to reduce the copy & paste source.

(In reply to Bobby Holley (:bholley) from comment #7)
> How many calls to enablePrivilege does the first patch remove, btw? I'm
> curious how far this takes us towards eliminating it entirely.

Removed 174 enablePrivilege calls :), added one call :(
(The count may be inaccurate because I counted it manually.)

Comment 9

[Bobby Holley (:bholley)]

(In reply to Masatoshi Kimura [:emk] from comment #8)
> (In reply to Bobby Holley (:bholley) from comment #5)
> > :::
> > toolkit/components/passwordmgr/test/test_basic_form_observer_autocomplete.
> > html
> > @@ +91,5 @@
> > >    is($_(2, "uname").value, "testuser", "Checking for filled username 2");
> > >    is($_(2, "pword").value, "testpass", "Checking for filled password 2");
> > >  
> > >    // Remove the observer
> > > +  //os.removeObserver(TestObserver, "passwordmgr-found-form");
> > 
> > Hm, why do we stop removing the observer here? Won't that cause us to leak?
> 
> removeObserver threw a cryptic error when I replaced enablePrivilege with
> SpecialPowers.

I think we should figure that out before landing. As it stands, this change will cause us to leak this dom window for the entire test run.

> 
> > ::: toolkit/components/prompts/test/test_bug620145.html
> > @@ +76,5 @@
> > >    e = document.createEvent("MouseEvent");
> > >    e.initEvent(type, false, false, win, 0, 1, 1, 1, 1,
> > >                false, false, false, false, 0, null);
> > >    var utils = SpecialPowers.getDOMWindowUtils(win);
> > > +  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> > 
> > Hm, why do we need this?
> 
> The test couldn't press the button in the alert without adding this. (Looks
> like the timeout handler was not executed at all during the alert.)

Hm, you could try using a different API to simulate the activity. I think we have a couple of different ways of doing this. 


> (In reply to Bobby Holley (:bholley) from comment #7)
> Removed 174 enablePrivilege calls :), added one call :(
> (The count may be inaccurate because I counted it manually.)

That's over 1/3 of the total in-tree enablePrivilege calls. Huzzah!

Comment 10

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1737d0cb675
https://hg.mozilla.org/integration/mozilla-inbound/rev/45446ab0c726
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8dac49d9dc

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Not yet landed part (passwordmgr/prompt)

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d1737d0cb675
https://hg.mozilla.org/mozilla-central/rev/45446ab0c726
https://hg.mozilla.org/mozilla-central/rev/fb8dac49d9dc

Comment 13

[Martijn Wargers (dead)]

The  "Not yet landed part (passwordmgr/prompt)"  patch probably doesn't apply anymore and I think a couple more enablePrivilege calls have been removed.

Masatoshi, what still needs to be done in this bug? 

> removeObserver threw a cryptic error when I replaced enablePrivilege with SpecialPowers.

I guess this was the part where you were stuck? And what the  "Not yet landed part (passwordmgr/prompt)" patch is about?

I filed bug 1149966 for removing the remaining enablePrivilege calls in mochitest.

Comment 14

[Masatoshi Kimura [:emk]]

I didn't investigate it further. Probably it's better to track the remaining removals on bug 1149966.