Closed Bug 1149966 Opened 6 years ago Closed 5 years ago

Remove enablePrivilege calls from remaining mochitest other than offline tests

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: martijn.martijn, Assigned: emk)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

Remove enablePrivilege calls from:
widget/tests/test_bug565392.html
uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml
toolkit/components/satchel/test/subtst_form_submission_1.html
toolkit/components/satchel/test/test_form_submission.html
dom/tests/mochitest/bugs/test_bug346659.html
dom/tests/mochitest/bugs/bug346659-parent-echoer.html
dom/tests/mochitest/bugs/bug346659-opener-echoer.html
dom/tests/mochitest/bugs/test_bug61098.html
embedding/test/test_bug449141.html (this is mochitest-chrome, so can just be removed)
js/xpconnect/tests/mochitest/test_bug504877.html

gfx/tests/crashtests/358732-iframe.html (crashtest, oops, forgot to remove this as part of bug 1145553)

Some of them are probably quite difficult to remove. It might be necessary to move them over to mochitest-chrome.

The removal of enablePrivilege of offline tests is covered in bug 871323.

Masatoshi, you were involved in bug 911573, removing a lot of the enablePrivilege calls, especially in passwordmanager.
Perhaps you know what needs to be done, regarding some of these tests? (I haven't tested just removing those, perhaps they can just be removed).
Flags: needinfo?(VYV03354)
Boris, is bug 346659 still testable after enablePrivilege is removed? SpecialPowers didn't bypass the cross origin restriction.
Flags: needinfo?(VYV03354) → needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(VYV03354)
> SpecialPowers didn't bypass the cross origin restriction.

Which exact cross origin restriction?

In any case, this is presumably convertable to a chrome test...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> Which exact cross origin restriction?

First, I simply removed enablePrivilege. The test failed with the following error:
> 5 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/bugs/test_bug346659.html | uncaught exception - Error: Permission denied to access property  "childWin" at handleCmd@http://mochi.test:8888/tests/dom/tests/mochitest/bugs/test_bug346659.html:46:5
> messageReceiver@http://mochi.test:8888/tests/dom/tests/mochitest/bugs/test_bug346659.html:87:7
> EventListener.handleEvent*@http://mochi.test:8888/tests/dom/tests/mochitest/bugs/test_bug346659.html:137:1

So I SpecialPowers.wrap()ed the cross-domain window object. But the test still failed with:
> 5 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/bugs/test_bug346659.html | uncaught exception - TypeError: SpecialPowers.wrap(...).childWin is undefined at handleCmd@http://mochi.test:8888/tests/dom/tests/mochitest/bugs/test_bug346659.html:46:5
> messageReceiver@http://mochi.test:8888/tests/dom/tests/mochitest/bugs/test_bug346659.html:87:7
> EventListener.handleEvent*@http://mochi.test:8888/tests/dom/tests/mochitest/bugs/test_bug346659.html:137:1

> In any case, this is presumably convertable to a chrome test...

I tried to port it to mochitest-chrome, but the test failed with:
> 2 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/bugs/test_bug346659.html | uncaught exception - SecurityError: The operation is insecure. at @chrome://mochitests/content/chrome/dom/tests/mochitest/bugs/test_bug346659.html:145:1

So I'm running out of the idea :(

According to bug 346659, this test is added in case the web page uses enablePrivilege. But it is no longer allowed. Is this test still necessary at all?
Flags: needinfo?(bzbarsky)
> First, I simply removed enablePrivilege. The test failed with the following error:

Sure, because then you can't touch things cross-domain.

> So I SpecialPowers.wrap()ed the cross-domain window object. But the test still failed with:

Does that still happen if you waive Xrays?  Or can you not get to the thing you need to waive Xrays on?

> I tried to port it to mochitest-chrome, but the test failed with:

In your ported version, what's on line 145?   Is it still the win.document.write() call where "win" comes from window.open("")?  If so, isn't the problem that you're changing that part of the test by opening a thing that's no longer same-origin with the opener, because you're opening it in a tab, not a chrome window, so we prevent the principal inheritance?

> this test is added in case the web page uses enablePrivilege

No.  It covers the case when the attacked page "has UniversalXPConnect or whatever".  System principal is "whatever" in this case.  ;)
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #4)
> > So I SpecialPowers.wrap()ed the cross-domain window object. But the test still failed with:
> 
> Does that still happen if you waive Xrays?  Or can you not get to the thing
> you need to waive Xrays on?

Oh, waiving did the trick. Thanks you.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9390ca9494c
Stop using enablePrivilege() in test_getUserMedia_scarySources.html r=jib
Got review in bug 1315858.
Keywords: leave-open
test_bug504877.html is a test for enablePrivilege itself. So I think we should not remove it until we remove enablePrivilege.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Flags: needinfo?(VYV03354)
Comment on attachment 8824653 [details]
Bug 1149966 - Remove enablePrivilege calls from remaining mochitest.

https://reviewboard.mozilla.org/r/103052/#review103636

really close, I would like these small things cleaned up prior to r+/landing.

::: dom/tests/mochitest/bugs/test_bug346659.html:41
(Diff revision 1)
>      // Not json
>      return false;
>    }  
>  
>    // Grab privileges so we can access cross-domain windows
> -  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> +  //netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

why is this commented out, can we just remove it?

::: dom/tests/mochitest/bugs/test_bug61098.html:23
(Diff revision 1)
>  <pre id="test">
>  </pre>
>  <script class="testbody" type="text/javascript">
>  /** Test for Bug 61098 **/
>  
> +function MockObjectRegisterer(aContractID, aReplacementCtor) {

why is mockObjects added inline here?

::: toolkit/components/satchel/moz.build:8
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  MOCHITEST_MANIFESTS += ['test/mochitest.ini']
> +#MOCHITEST_CHROME_MANIFESTS += ['test/chrome.ini']

can we remove this line if we are not using it?  I would rather put a skip-if in the chrome.ini with a corresponding bug #.
Attachment #8824653 - Flags: review?(jmaher) → review-
Comment on attachment 8824653 [details]
Bug 1149966 - Remove enablePrivilege calls from remaining mochitest.

https://reviewboard.mozilla.org/r/103052/#review103636

> why is this commented out, can we just remove it?

No reason, removed.

> why is mockObjects added inline here?

MockObjects.js is used by other chrome tests. I had to change the code a bit to make it usable from content.
I merged the change to the MockObjects.js.

> can we remove this line if we are not using it?  I would rather put a skip-if in the chrome.ini with a corresponding bug #.

It was wreckage of an attempt to port test_form_submission.html. The test was not ported, after all. So I removed chrome.ini.
Comment on attachment 8824653 [details]
Bug 1149966 - Remove enablePrivilege calls from remaining mochitest.

https://reviewboard.mozilla.org/r/103052/#review103676

::: widget/tests/chrome.ini:21
(Diff revision 2)
>  [test_bug517396.xul]
>  [test_bug538242.xul]
>  support-files = window_bug538242.xul
> +[test_bug565392.html]
> +subsuite = clipboard
> +skip-if = toolkit != "windows"

if https://bugzilla.mozilla.org/show_bug.cgi?id=1267406 is fixed, please close it, otherwise we should add in:
|| e10s # bug 1267406
Attachment #8824653 - Flags: review?(jmaher) → review-
Comment on attachment 8824653 [details]
Bug 1149966 - Remove enablePrivilege calls from remaining mochitest.

https://reviewboard.mozilla.org/r/103052/#review103676

> if https://bugzilla.mozilla.org/show_bug.cgi?id=1267406 is fixed, please close it, otherwise we should add in:
> || e10s # bug 1267406

I closed bug 1267406. mochitest-chrome will only run in non-e10s.
Attachment #8824653 - Flags: review- → review?
Attachment #8824653 - Flags: review? → review?(jmaher)
Comment on attachment 8824653 [details]
Bug 1149966 - Remove enablePrivilege calls from remaining mochitest.

https://reviewboard.mozilla.org/r/103052/#review103798

thanks!
Attachment #8824653 - Flags: review?(jmaher) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/27f8d8e9c4dc
Remove enablePrivilege calls from remaining mochitest. r=jmaher
Hm. I used postMessage to get around cross-origin restriction in test_form_submission.html.
When subtst_form_submission_1.html posts back a message for test 21, the form submission is still in progress. If the form submission does not complete until the parent frame posts a message for test 100, no message handler will be attached in iframe and the message will be discarded, hence timeout.
Comment on attachment 8824653 [details]
Bug 1149966 - Remove enablePrivilege calls from remaining mochitest.

I stopped using postMessage in test_form_submissions.html. The test no longer timeouts on Win7 VM:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=430b92e1e542f3e6dfbecd84a62a65838105a864&filter-searchStr=Windows%207%20VM
Attachment #8824653 - Flags: review+ → review?(jmaher)
Comment on attachment 8824653 [details]
Bug 1149966 - Remove enablePrivilege calls from remaining mochitest.

https://reviewboard.mozilla.org/r/103052/#review104318

thanks for updating this- I don't see anything scary in this version.
Attachment #8824653 - Flags: review?(jmaher) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/6134feaa6eb7
Remove enablePrivilege calls from remaining mochitest. r=jmaher
Keywords: leave-open
Duplicate of this bug: 750638
https://hg.mozilla.org/mozilla-central/rev/6134feaa6eb7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
This is awesome work! Thanks for doing it.
You need to log in before you can comment on or make changes to this bug.