Closed Bug 901343 Opened 6 years ago Closed 6 years ago

Tests that use createSystemXHR are failing on b2g mochitest

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch 338583.diff (obsolete) — Splinter Review
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
Attachment #785510 - Flags: feedback?(jmaher)
Hrm, test_xhr_forbidden_headers.html seems to work just fine in b2g mochitest.
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.
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.
(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.
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
Attachment #785510 - Flags: feedback?(jmaher) → feedback+
(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.
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?
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...
Ok, I filed bug 902611 for the test_bug475156.html test failing.
Assignee: nobody → martijn.martijn
>  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.
(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!
(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)?
Attached patch 901343.diff (obsolete) — Splinter Review
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=6f8e7dfeb7e
Attachment #785510 - Attachment is obsolete: true
1 orange, but that's because of a stupid mistake in test_SpecialPowersExtension.html.
Attached patch 901343.diff (obsolete) — Splinter Review
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.
Attachment #788721 - Attachment is obsolete: true
Attachment #788733 - Flags: review?(jonas)
Attachment #788733 - Flags: review?(jonas)
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.
Attached patch 901343.diff (obsolete) — Splinter Review
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=0a7f71f45120
Attachment #788733 - Attachment is obsolete: true
Attachment #789090 - Flags: review?(jonas)
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.
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?
Attachment #789090 - Flags: review?(jonas) → review-
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
(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.
Oh, and SpecialPowers.pushPermissions needs a callback, so I can't simply adjust the createSystemXHR-call and return something different for b2g.
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.
Flags: needinfo?(jonas)
Updated the patch to current, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b216ef7dfd86
Attachment #789090 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #816174 - Attachment description: 901343.diff → 901343.diff (for check-in)
https://hg.mozilla.org/integration/fx-team/rev/1a5a56c79e3c
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1a5a56c79e3c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
(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.
You need to log in before you can comment on or make changes to this bug.