Get dom/security/test/csp/test_report.html and test_blocked_uri_in_reports.html working with e10s

RESOLVED FIXED in Firefox 45

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
test_blocked_uri_in_reports.html is basically the same thing, so I can fix it at the same time.
Summary: Get dom/security/test/csp/test_report.html working with e10s → Get dom/security/test/csp/test_report.html and test_blocked_uri_in_reports.html working with e10s
(Assignee)

Comment 2

2 years ago
Created attachment 8683988 [details] [diff] [review]
Make test_report.html and test_blocked_uri_in_reports.html work with e10s.

This moves the code that analyzes the netwerking stuff into the parent process. It then sends the string down to the child for analysis, or an error if there was one. It looks like that part of the code is identical between these two tests so it has the nice benefit of reducing the over all size of the tests.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d7f5729a60b
Attachment #8683988 - Flags: review?(mozilla)
Comment on attachment 8683988 [details] [diff] [review]
Make test_report.html and test_blocked_uri_in_reports.html work with e10s.

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

Nicely done, that means that all CSP tests are enabled for e10s now - which is great - thanks Andrew.

::: dom/security/test/csp/test_blocked_uri_in_reports.html
@@ +51,5 @@
> +      // blocked-uri should only be the asciiHost instead of:
> +      // http://test1.example.com/tests/dom/security/test/csp/file_path_matching.js
> +      is(cspReport["blocked-uri"], "http://test1.example.com", "Incorrect blocked-uri");
> +    } catch (e) {
> +      ok(false, "Could not query report (exception: " + e + ")");

same nit applies as for test_report.html

::: dom/security/test/csp/test_report.html
@@ +78,5 @@
> +    try {
> +      // test for the proper values in the report object
> +      window.checkResults(reportObj);
> +    } catch (e) {
> +      ok(false, "Could not query report (exception: " + e + ")");

Super nit: I usually prefer early returns. Would you mind fixing that up so whenever we encounter an error or end up in a catch block we log the error and return early?
Attachment #8683988 - Flags: review?(mozilla) → review+
(Assignee)

Comment 4

2 years ago
The drawback of the early return is that some code will have to be added before every return:
  script.removeMessageListener('opening-request-completed', ml);
  SimpleTest.finish();
I guess the removeMessageListener can go at the top of the message listener, but the SimpleTest.finish() still has to go before the returns. Is that okay?
Flags: needinfo?(mozilla)
(In reply to Andrew McCreight [:mccr8] from comment #4)
> The drawback of the early return is that some code will have to be added
> before every return:
>   script.removeMessageListener('opening-request-completed', ml);
>   SimpleTest.finish();
> I guess the removeMessageListener can go at the top of the message listener,
> but the SimpleTest.finish() still has to go before the returns. Is that okay?

Ah, I haven't realized that that code is still before the closing curly brace. In that case I would say, just keep what you have. It was only a nitpicky thing anyway.
Flags: needinfo?(mozilla)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff2125aab1c2
Keywords: checkin-needed

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff2125aab1c2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.