986077 - Enable CSP tests on b2g-desktop, b2g-debug and e10s

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Garrett Robinson [:grobinson]]

When I landed bug 945268, I didn't realize there were different lists of blocked tests for the various versions of B2G (device, emulator, device debug, emulator debug, b2g desktop, b2g desktop debug, etc.), and only enabled the tests on non-debug emulator and device builds.

This situation has been improved somewhat by a new mochitest.ini introduced recently, which unifies these exceptions locally. See bug 984716 for background. Many of the CSP tests remain blocked on B2G debug and desktop builds, but unnecessarily - the underlying causes were fixed in bug 945268. This bug is to get the CSP tests running on as many versions of B2G as possible, leveraging preexisting work.

Comment 1

[Garrett Robinson [:grobinson]]

I was able to get all but 2 of the tests running locally on a B2G-desktop debug build.

The two tests that still don't work are:

1. test_CSP_evalscript_getCRMFRequest.html

This test is not expected to work - the window.crypto functions it is testing are deprecated, and are not supported in any multiprocess Gecko. Since they functions it is testing are not supported, there is no issue with disabling this test.

2. test_CSP_frameancestors.html

This test works elsewhere (desktop e10s, B2G emulator), but it appeared to time out when I ran it locally. I am leaving it disabled for now, but I'm not sure what's causing the discrepancy.

Try: https://tbpl.mozilla.org/?tree=Try&rev=46842bbe3fa2

Comment 2

[Garrett Robinson [:grobinson]]

Some tests ran locally on B2G-desktop, but failed on try (B2G ICS Emulator Opt). Not sure why there is a discrepancy there - do B2G-desktop and other B2G use different process models?

Comment 3

[Garrett Robinson [:grobinson]]

I found some tests that should have been converted in bug 845268, but were not. These are fixed in latest patch. This leaves at least two tests that still fail on B2G ICS, and may be difficult/impossible to get running, at least soon:

1. test_csp_report.html

Uses http-on-opening-request (and the nsIHttpChannel subject) quite heavily, not amenable to the simple "proxy the URI of the load" method used elsewhere to convert the tests for multiprocess.

2. test_hash_source.html

Not confirmed yet, but almost certainly due to bug 958702.

These test failures are due to fundamental changes in the way common infrastructure, like Necko/Gecko interaction and the nsIObserverService, have changed due to multiprocess/e10s. Fixing them may make sense than "fixing" CSP (i.e., enforcing it in the parent process instead of in the child).

Comment 4

[Garrett Robinson [:grobinson]]

Attachment: 986077-csp-tests-b2g-debug-and-desktop.patch

Enables all CSP tests that ran locally on B2G-desktop.

Fixes tests that should have been converted before, but were not. They were probably pref'ed off the first time around.

Comment 5

[Jonathan Griffin (:jgriffin)]

(In reply to Garrett Robinson [:grobinson] from comment #2)
> Some tests ran locally on B2G-desktop, but failed on try (B2G ICS Emulator
> Opt). Not sure why there is a discrepancy there - do B2G-desktop and other
> B2G use different process models?

Yes, B2G desktop builds are currently not OOP, but emulator builds are.

Comment 6

[Garrett Robinson [:grobinson]]

I re-ran the csp mochitest suite on b2g-desktop with this patch applied, and everything passed. 

test_CSP_frameancestors.html is still timing out for an unknown reason, but I don't want to block on that so I filed a followup, bug 1008445.

Try: https://tbpl.mozilla.org/?tree=Try&rev=b192f9db41cc

Comment 7

[Garrett Robinson [:grobinson]]

This blocks 988616 because I am basing that patch on this one. I don't want to have to do this work twice.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

As long as you're doing this Garrett, would you mind trying to remove this line:
http://mxr.mozilla.org/mozilla-central/source/content/base/test/csp/mochitest.ini#2
Then we would run these tests on desktop e10s as well. You can try it out by running mochitests as normal but with the --e10s option.

Comment 9

[Garrett Robinson [:grobinson]]

Attachment: 986077-csp-tests-b2g-debug-and-desktop.patch

Enable as many tests as possible on e10s. Try: https://tbpl.mozilla.org/?tree=Try&rev=46b90e7941f0

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8420439 [details] [diff] [review]
986077-csp-tests-b2g-debug-and-desktop.patch

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

Hey Garret, this looks good. I am going to r- this feedback request just because of my comments in mochitest.ini. I think once this lands we will never go back and fix those tiny missing parts up. I can help you investigate. Otherwise your changes look good an ready to go.

::: content/base/test/csp/mochitest.ini
@@ +137,2 @@
>  [test_CSP_frameancestors.html]
> +skip-if = (buildapp == 'b2g' && (toolkit != 'gonk' || debug)) || toolkit == 'android' # Times out, not sure why (bug 1008445)

Maybe we should investigate bug 1008445 again - I can also take a look if you want.

@@ +150,2 @@
>  [test_hash_source.html]
> +skip-if = e10s # can't compute hashes in child process (bug 958702)

Hm, this is the only test that checks if noncePreload is working or not. Is it worth splitting the test, so that the noncePreload is running on e10s and just not the hash-part? Maybe worth to think about it.

@@ +154,5 @@
>  [test_bug949549.html]
>  [test_csp_regexp_parsing.html]
>  [test_report_uri_missing_in_report_only_header.html]
>  [test_csp_report.html]
> +skip-if = e10s # http-on-opening-request observer not supported in child process

How come we have specialpowers-http-notify-request, but not a specialpower equivalent for http-on-opnening-request?

::: content/base/test/csp/test_multi_policy_injection_bypass.html
@@ +53,2 @@
>        //these things were allowed by CSP
> +      dump("** data: " + data + "\n");

You can delete that dump.

@@ +64,5 @@
>      if(topic === "csp-on-violate-policy") {
> +      // subject should be an nsIURI for csp-on-violate-policy
> +      if(!SpecialPowers.can_QI(subject))
> +        return;
> +

Nit: I do like {} even around one line if-statements.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8420439 [details] [diff] [review]
986077-csp-tests-b2g-debug-and-desktop.patch

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

lgtm with ckerschb's comments addressed.  Feel free to re-flag if you want me to look again after a revision (since ckerschb wants to see it again).  The html file changes look good based on what we did for other special-powers-based observers of http-on-modify-request.

Comment 12

[Garrett Robinson [:grobinson]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> Hey Garret, this looks good. I am going to r- this feedback request just
> because of my comments in mochitest.ini. I think once this lands we will
> never go back and fix those tiny missing parts up. I can help you
> investigate. Otherwise your changes look good an ready to go.

We will follow up on these issues, that's why I filed bugs for them. I believe the few remaining tests that need fixing are non-trivial, and I'd rather:

1. Dramatically increase our test coverage on B2G and e10s ASAP
2. Not block landing the CSP rewrite, since this bug blocks bug 988616

You could argue that this bug doesn't *need* to block 988616, but I'd rather do this work one time if possible.

> ::: content/base/test/csp/mochitest.ini
> @@ +137,2 @@
> >  [test_CSP_frameancestors.html]
> > +skip-if = (buildapp == 'b2g' && (toolkit != 'gonk' || debug)) || toolkit == 'android' # Times out, not sure why (bug 1008445)
> 
> Maybe we should investigate bug 1008445 again - I can also take a look if
> you want.

That would be great. test_CSP_frameancestors.html is a complex test. It would be good to get one of the test's authors (sstamm or mwargers) [0] to take a look as well.

[0] http://hg.mozilla.org/mozilla-central/annotate/7f5a8526b55a/content/base/test/csp/test_CSP_frameancestors.html

> @@ +150,2 @@
> >  [test_hash_source.html]
> > +skip-if = e10s # can't compute hashes in child process (bug 958702)
> 
> Hm, this is the only test that checks if noncePreload is working or not. Is
> it worth splitting the test, so that the noncePreload is running on e10s and
> just not the hash-part? Maybe worth to think about it.

Incorrect, test_nonce_source.html tests noncePreload.

The problem here is larger than the test being broken - the hash-source *feature* is broken on multiprocess Gecko. This is a known issue with using NSS for the hash functions, and the followup bug has more details. Resolving it is nontrivial.

> @@ +154,5 @@
> >  [test_bug949549.html]
> >  [test_csp_regexp_parsing.html]
> >  [test_report_uri_missing_in_report_only_header.html]
> >  [test_csp_report.html]
> > +skip-if = e10s # http-on-opening-request observer not supported in child process
> 
> How come we have specialpowers-http-notify-request, but not a specialpower
> equivalent for http-on-opnening-request?

Missed it when I wrote the specialpowers proxy. This is the only test that uses on-opening-request. I'm not sure if that's necessary, on-modify-request might work too. The difference here is that we also need to get the contents of the request and pass it with the proxy. This is probably straightforward, but I'd rather track it in a follow-up than block on it for the reasons above.

This reminded me that I forgot to file a followup for this particular issue, so I've done that now (bug 1009632), and updated the comment in mochitest.ini.

> ::: content/base/test/csp/test_multi_policy_injection_bypass.html
> @@ +53,2 @@
> >        //these things were allowed by CSP
> > +      dump("** data: " + data + "\n");
> 
> You can delete that dump.

Done.

> @@ +64,5 @@
> >      if(topic === "csp-on-violate-policy") {
> > +      // subject should be an nsIURI for csp-on-violate-policy
> > +      if(!SpecialPowers.can_QI(subject))
> > +        return;
> > +
> 
> Nit: I do like {} even around one line if-statements.

Done.

Comment 13

[Garrett Robinson [:grobinson]]

Attachment: 986077-csp-tests-b2g-debug-and-desktop.patch

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8421807 [details] [diff] [review]
986077-csp-tests-b2g-debug-and-desktop.patch

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

Thanks for clarification Garrett! This is ready to go!!!

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks Garrett!

Comment 16

[Garrett Robinson [:grobinson]]

Attachment: 986077-csp-tests-b2g-debug-and-desktop.patch

Needed to disable test_hash_source and test_csp_reports on the B2G emulator as well (they don't run on multiprocess, which is e10s and B2G emulator, but not B2G desktop which is single process).

Try: https://tbpl.mozilla.org/?tree=Try&rev=7268839c572c

Comment 17

[Garrett Robinson [:grobinson]]

Attachment: 986077-csp-tests-b2g-debug-and-desktop.patch

Tweak flags to stop tests from running on the B2G emulator. Try: https://tbpl.mozilla.org/?tree=Try&rev=133863dd062d

Comment 18

[Garrett Robinson [:grobinson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a28cb66df977

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/a28cb66df977