Fix race condition within wpt test policy-inherited-correctly-by-plznavigate.html

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
7 months ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [domsecurity-active][wptsync upstream])

Attachments

(2 attachments, 1 obsolete attachment)

As discussed within [1] the srcdoc frame might not be loaded before the document.write happens, we should fix that race condition!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=965637#c43
Assignee: nobody → ckerschb
Blocks: 965637
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Attachment #8983030 - Flags: review?(james) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c71a38c8c5c
Fix race condition within wpt test policy-inherited-correctly-by-plznavigate.html. r=jgraham
Keywords: checkin-needed
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11325 for changes under testing/web-platform/tests
Whiteboard: [domsecurity-active] → [domsecurity-active][wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
The new test is racy in a different way: The code in the script can run _after_ the load event from the frame has fired, in which case none of the test will get run....
Flags: needinfo?(james)
Flags: needinfo?(ckerschb)
Upstream PR merged
https://hg.mozilla.org/mozilla-central/rev/0c71a38c8c5c
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Status: RESOLVED → REOPENED
Flags: needinfo?(james)
Flags: needinfo?(ckerschb)
Resolution: FIXED → ---
Boris, thanks for taking another look here. Here is a new version, what do you think?
Attachment #8983166 - Flags: review?(bzbarsky)
Comment on attachment 8983166 [details] [diff] [review]
bug_1466508_fix_race_within_wpt_test_part2.patch

This looks better, yes.

That said, do we need to make sure that the <script async defer> bit runs _after_ all this stuff?  I haven't looked into exactly what that does, but I'm a bit suspicious.
Flags: needinfo?(ckerschb)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> That said, do we need to make sure that the <script async defer> bit runs
> _after_ all this stuff?  I haven't looked into exactly what that does, but
> I'm a bit suspicious.

Boris, most of the web platform tests simply use <script async defer ...> for testing the CSP error reporting within a test. I looked through the various wpt tests in our tree and also found [1] which generates that script using JS. Since we are already on this test file, we might also just do the same to make sure there is no race within this test.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/object-src/object-src-2_2.html#47-50
Attachment #8983166 - Attachment is obsolete: true
Attachment #8983166 - Flags: review?(bzbarsky)
Flags: needinfo?(ckerschb)
Attachment #8983340 - Flags: review?(bzbarsky)
> most of the web platform tests simply use <script async defer ...> for testing the CSP error
> reporting within a test.

Right.  Is that supported by the spec?  That is, does the spec require such a script to run after the CSP reporting bits have happened?
Comment on attachment 8983340 [details] [diff] [review]
bug_1466508_fix_race_within_wpt_test_part2.patch

r=me
Attachment #8983340 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> > most of the web platform tests simply use <script async defer ...> for testing the CSP error
> > reporting within a test.
> 
> Right.  Is that supported by the spec?  That is, does the spec require such
> a script to run after the CSP reporting bits have happened?

Sorry, I think I can't follow what you mean. The wpt tests use a script to query information from a pseudo server to check if CSP reports were send correctly. The spec only requires policy reports to be send when a resource load was blocked by CSP, not in what order that needs to happen.
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fac57eb7e7aa
Fix race condition within wpt test policy-inherited-correctly-by-plznavigate.html part 2. r=bz
> The wpt tests use a script to query information from a pseudo server to check if CSP reports were send correctly.

Right.  My question is whether the spec requires the reports to be sent and received on the server before this script would hit the server with its query.  If not, then the tests are all racy and we should get a bug filed on them upstream or something...
Flags: needinfo?(ckerschb)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> > The wpt tests use a script to query information from a pseudo server to check if CSP reports were send correctly.
> 
> Right.  My question is whether the spec requires the reports to be sent and
> received on the server before this script would hit the server with its
> query.  If not, then the tests are all racy and we should get a bug filed on
> them upstream or something...

I am pretty sure all those tests suffer from that race condition. I filed Bug 1467155 to get that updated and fixed.
Flags: needinfo?(ckerschb)
https://hg.mozilla.org/mozilla-central/rev/fac57eb7e7aa
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11416 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.