Martijn Wargers (dead) Assignee
	Description • 11 years ago

For instance this test: http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug338583.html?force=1
It is currently failing in b2g mochitest with this error:
7 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug338583.html | uncaught exception - NS_ERROR_ILLEGAL_VALUE: Illegal value at chrome://specialpowers/content/specialpowersAPI.js:88

I noticed that I could use something like SpecialPowers.addPermission("systemXHR", true, document) and new XMLHttpRequest({mozSystem: true}); instead of SpecialPowers.createSystemXHR().
Can this be used instead for the tests?
The patch is a poc, if you approve this way, I can rewrite all these tests this way:
test_bug431701.html 
test_bug338583.html
test_xhr_forbidden_headers.html 
test_bug426308.html
test_bug804395.html
test_SpecialPowersExtension.html

If this is possible, it seems that SpecialPowers.createSystemXHR() is not really necessary?

I still get 3 failures with this test on b2g mochitest, but those are 'regular' test failures and all the tests are at least running in the file (42).

Btw, I noticed on desktop, that this test would trigger these errors in the console afterwards, continuously:
System JS : ERROR (null):0
                     uncaught exception: 2147746065

	Martijn Wargers (dead) Assignee
	Comment 1 • 11 years ago

Hrm, test_xhr_forbidden_headers.html seems to work just fine in b2g mochitest.

	Martijn Wargers (dead) Assignee
	Comment 2 • 11 years ago

Ok, I think this is because test_xhr_forbidden_headers.html  isn't using xhr.send().
In b2g land, createSystemXHR doesn't really create a systemXHR object, since it's called from content.

	Martijn Wargers (dead) Assignee
	Comment 3 • 11 years ago

Here is the createSystemXHR function: http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#1123
This makes use of the SpecialPowers.wrap() function. 

It reminds me of the issue I was seeing in bug 865204, comment 13.

From bug 865204, comment 12:
"SpecialPowers.wrap(foo) just gives you a js-proxy to foo that forwards any operations you do so that they happen in a system-principal-ed scope. It's basically just a one-size-fits-all replacement for stuff like SpecialPowers.do_QueryInterface and SpecialPowers.getPrivilegedProps."

It seems that can't be the case for b2g mochitests, right? Because the tests are in the content process, so those operations can't be forwarded to a system-principal-ed scope. Or am I wrong?

I'm seeing a similar failure happening in test_bug475156.html and I suspect that there are quite a few more tests that use SpecialPowers.wrap().
So it would be great if SpecialPowers.wrap() could be working similarly in b2g mochitests as on desktop mochitests.

	Bobby Holley (:bholley)
	Comment 4 • 11 years ago

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #3)
> Here is the createSystemXHR function:
> http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/
> specialpowersAPI.js#1123
> This makes use of the SpecialPowers.wrap() function. 
> 
> It reminds me of the issue I was seeing in bug 865204, comment 13.
> 
> From bug 865204, comment 12:
> "SpecialPowers.wrap(foo) just gives you a js-proxy to foo that forwards any
> operations you do so that they happen in a system-principal-ed scope. It's
> basically just a one-size-fits-all replacement for stuff like
> SpecialPowers.do_QueryInterface and SpecialPowers.getPrivilegedProps."
> 
> It seems that can't be the case for b2g mochitests, right? Because the tests
> are in the content process, so those operations can't be forwarded to a
> system-principal-ed scope. Or am I wrong?

System-principal-ed scopes exist in content processes.

> I'm seeing a similar failure happening in test_bug475156.html and I suspect
> that there are quite a few more tests that use SpecialPowers.wrap().
> So it would be great if SpecialPowers.wrap() could be working similarly in
> b2g mochitests as on desktop mochitests.

It should work the same, in theory. But depending on what you're using SpecialPowers.wrap to _do_ it may not work. For example, if you try to use SpecialPowers to do some QI magic and access the browser element containing the page, it's not going to work.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 5 • 11 years ago

Comment on attachment 785510 [details] [diff] [review]
338583.diff

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

I assume this has been tested and proven to work.  I am not that familiar with the content process or security model of how tests are run on b2g.  This doesn't look too harmful, but my biggest concern is running test case on desktop.  Since this is for feedback I am happy with a f+, for review I would need to really see the impact this has on desktop tests.

::: content/base/test/test_bug338583.html
@@ +461,5 @@
>  
>    function doTest5_c(test_id)
>    {
>      // credentials using the auth cache and cookies
> +    var xhr = new XMLHttpRequest({mozSystem: true});

according to the MDN docs, this requires setting mozAnon=true:
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest?redirectlocale=en-US&redirectslug=DOM%2FXMLHttpRequest#XMLHttpRequest%28%29

	Martijn Wargers (dead) Assignee
	Comment 6 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #4)
> It should work the same, in theory. But depending on what you're using
> SpecialPowers.wrap to _do_ it may not work. For example, if you try to use
> SpecialPowers to do some QI magic and access the browser element containing
> the page, it's not going to work.

I understand that accessing the browser is not going to work, but in this case:
1123   createSystemXHR: function() {
1124     return this.wrap(Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest));
1125   },

That's just trying to instatiate an instance of nsIXMLHttpRequest. That works, but then trying to use a method like .open() gives me these NS_ERROR_ILLEGAL_VALUE errors.
There are more failures like this, where SpecialPowers.wrap() is involved.
I'll come up with a list of that, later today.

	Martijn Wargers (dead) Assignee
	Comment 7 • 11 years ago

Ok, it turns out that are not that much more failures like this. Only in 4 case total:
* content/base/test/test_bug338583.html and content/base/test/test_bug804395.html are failing because of createsystemxhr usage.
* content/base/test/test_bug475156.html is failing with (NS_NOINTERFACE [nsISupports.QueryInterface]", which is from this code:
20   var ch = SpecialPowers.wrap(xhr).channel.QueryInterface(SpecialPowers.Ci.nsICachingChannel);
* content/base/test/test_bug422403-1.html is on an nsIChannel.open call which was made from SpecialPowers.

The part in SpecialPowersAPI.js where it is failing, btw, is this:
87 function doApply(fun, invocant, args) {
88   return Function.prototype.apply.call(fun, invocant, args);
89 }

These 4 failures are all related to networking, I guess because those are run in a less privileged process or something like that?

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 11 years ago

Well, necko in a child process acts a bit differently... e.g. HttpChannelChild does not in fact QI to nsICachingChannel.  So at least that test will need fixing to work in an e10s environment...

	Martijn Wargers (dead) Assignee
	Comment 9 • 11 years ago

Ok, I filed bug 902611 for the test_bug475156.html test failing.

	Jason Duell
	Comment 10 • 11 years ago

>  e.g. HttpChannelChild does not in fact QI to nsICachingChannel

Yes, we deliberately split out a subset of the the nsICachingChannel features into nsICacheInfoChannel, which is all that you get on the child side at the moment.

	Martijn Wargers (dead) Assignee
	Comment 11 • 11 years ago

(In reply to Jason Duell (:jduell) from comment #10)
> Yes, we deliberately split out a subset of the the nsICachingChannel
> features into nsICacheInfoChannel, which is all that you get on the child
> side at the moment.

Thanks! That works very well!

	Martijn Wargers (dead) Assignee
	Comment 12 • 11 years ago

(In reply to Joel Maher (:jmaher) from comment #5)
> according to the MDN docs, this requires setting mozAnon=true:
> https://developer.mozilla.org/en-US/docs/Web/API/
> XMLHttpRequest?redirectlocale=en-
> US&redirectslug=DOM%2FXMLHttpRequest#XMLHttpRequest%28%29

I get failures in content/base/test/test_bug338583.html when adding mozAnon: true to the poc patch. With mozAnon: false, I don't get failures. This documentation in mdn is based on bug 692677, comment 68. Jonas made that comment. Jonas, is it really necessary to add mozAnon: true (while mozAnon: false doesn't cause failures)?

	Martijn Wargers (dead) Assignee
	Comment 13 • 11 years ago

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=6f8e7dfeb7e

	Martijn Wargers (dead) Assignee
	Comment 14 • 11 years ago

1 orange, but that's because of a stupid mistake in test_SpecialPowersExtension.html.

	Martijn Wargers (dead) Assignee
	Comment 15 • 11 years ago

Fixed the stupid mistake and apart from that orange that resulted from it, all looks green, so this would allow these tests to run on b2g.

	Martijn Wargers (dead) Assignee
	Comment 16 • 11 years ago

Ok, it turns out that test_bug431701.html is causing b2b to crash, which is bug 902271.
With test_bug338583.html, I get "Failed to set credentials" in test 5.c and test 5d on b2g, which seems to be related to the use of https.

	Martijn Wargers (dead) Assignee
	Comment 17 • 11 years ago

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=0a7f71f45120

	Martijn Wargers (dead) Assignee
	Comment 18 • 11 years ago

Ok, test_bug338583.html is also failing with the same errors as mentioned in comment 16 on Android, so it has to stay disabled there as well.
I filed bug 904257 for this issue.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 19 • 11 years ago

Comment on attachment 789090 [details] [diff] [review]
901343.diff

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

::: content/base/test/test_bug338583.html
@@ +464,5 @@
>  
>    function doTest5_c(test_id)
>    {
>      // credentials using the auth cache and cookies
> +    var xhr = new XMLHttpRequest({mozAnon: false, mozSystem: true});

I'm not really a fan of changing this. It doesn't seem to scale across all of our tests, especially with the need for added permissions.

Could you make SpecialPowers.createSystemXHR() use a separate implementation in B2G which calls SpecialPowers.pushPermissions just for the lifetime of the createSystemXHR-call and then creates a XHR({anon, system}) and returns that?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 20 • 11 years ago

In general, SpecialPowers should be abstracting over the different platforms that we have. It's not good if some SpecialPowers functions work on some platforms but not others as it'll lead to people keep adding tests that doesn't work for B2G

	Martijn Wargers (dead) Assignee
	Comment 21 • 11 years ago

(In reply to Jonas Sicking (:sicking) from comment #20)
> In general, SpecialPowers should be abstracting over the different platforms
> that we have. It's not good if some SpecialPowers functions work on some
> platforms but not others as it'll lead to people keep adding tests that
> doesn't work for B2G

Yes, that's why I removed the SpecialPowers.createSystemXHR() function completely.
I see the SpecialPowers functions needed for things that are needed for tests, but can't be done with regular javascript.
SpecialPowers.createSystemXHR() doesn't seem to fit that as one can do the same with XHR({anon, system}), apparently.

(In reply to Jonas Sicking (:sicking) from comment #19)
> I'm not really a fan of changing this. It doesn't seem to scale across all
> of our tests, especially with the need for added permissions.

The systemXHR permission can be enabled permanently for all mochitests, that way, you wouldn't have to add any 'systemXHR' permission (only when you want to see it disabled).

> Could you make SpecialPowers.createSystemXHR() use a separate implementation
> in B2G which calls SpecialPowers.pushPermissions just for the lifetime of
> the createSystemXHR-call and then creates a XHR({anon, system}) and returns
> that?

I could do that, but I would prefer not to follow different code paths for b2g, if it's not necessary. In this case, it doesn't seem necessary, afaict.
Is there a difference between SpecialPowers.createSystemXHR() and XHR({anon, system})? If there isn't, why follow different code paths?
If there is, and tests are exercising it, then it would cause a test failure in b2g, and I would have to fix those tests with another different code path.

	Martijn Wargers (dead) Assignee
	Comment 22 • 11 years ago

Oh, and SpecialPowers.pushPermissions needs a callback, so I can't simply adjust the createSystemXHR-call and return something different for b2g.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 23 • 11 years ago

Chatted with Martijn and I think this patch is the best approach that we can take currently.

Ideally I'd like to create an XHR object with a system principal, but with the webpage window as the global, but there is no way to do that right now.

	Martijn Wargers (dead) Assignee
	Comment 24 • 11 years ago

Updated the patch to current, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b216ef7dfd86

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/1a5a56c79e3c

	Phil Ringnalda (:philor)
	Comment 26 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/1a5a56c79e3c

	Martijn Wargers (dead) Assignee
	Comment 27 • 11 years ago

(In reply to Martijn Wargers [:mwargers] (QA) from comment #12)
> I get failures in content/base/test/test_bug338583.html when adding mozAnon:
> true to the poc patch. With mozAnon: false, I don't get failures. This
> documentation in mdn is based on bug 692677, comment 68. Jonas made that
> comment. Jonas, is it really necessary to add mozAnon: true (while mozAnon:
> false doesn't cause failures)?

I filed bug 927196 for this now. I don't see the failures now anymore with mozAnon: true, so I can change it back to that, but there is still the problem that mozAnon: false shouldn't work at all in combination with mozSystem: true.