Closed Bug 1364367 Opened 3 years ago Closed 3 years ago

rewrite test_bug379959.html

Categories

(Core :: DOM: Security, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 1 obsolete file)

test_bug379959.html is considering data: URI as the same origin, we should revise it.
Attached patch Patch. (obsolete) — Splinter Review
Attachment #8867091 - Flags: review?(ckerschb)
Comment on attachment 8867091 [details] [diff] [review]
Patch.

Yoshi, here is what I think we need to do: In those cases where we are actually test data: URI inheritance behavior like in the specific case of this bug, we need to clone the test. In more detail:

1) Keep that test as is but *explicitly* use:
>  SpecialPowers.setBoolPref("security.data_uri.inherit_security_context", true);

2) Clone that test (call it something like test_bug379959_data_not_inherits.html), use:
>  SpecialPowers.setBoolPref("security.data_uri.inherit_security_context", false);
and change the test like you did within this patch.

This way we will always have test coverage for both versions (data inherits and data does not inherit) depending on the actual value of the pref. Once we have flipped the pref and are confident that everything works as expected we can eliminate all the 'old' tests.
Flags: needinfo?(allstars.chh)
Attachment #8867091 - Flags: review?(ckerschb)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Comment on attachment 8867091 [details] [diff] [review]
> Patch.
> 
> Yoshi, here is what I think we need to do: In those cases where we are
> actually test data: URI inheritance behavior like in the specific case of
> this bug, we need to clone the test. In more detail:
> 
> 1) Keep that test as is but *explicitly* use:
> >  SpecialPowers.setBoolPref("security.data_uri.inherit_security_context", true);
I am not sure is it worth doing this?
Data URI should NOT inherit the origin per W3C spec, that's why we are fixing it now.

Why do we still need to spend time to maintain the 'WRONG' behavior?
Given that our priority should be 
1. To use the CORRECT behavior (bug 1324406)
2. To add tests to test the CORRECT behavior (bug 1365145)

And to be honest, the two items above will take a lots of time and effort, now we still spend more time to maintain old one, and once we fix all of them we also need to spend time to remove old ones....

I understand your concern but it doesn't seem to be so realistic to me.
Flags: needinfo?(allstars.chh)
(In reply to Yoshi Huang [:allstars.chh] from comment #3)
> Why do we still need to spend time to maintain the 'WRONG' behavior?

Yoshi, as discussed in our meeting have to go down that route. In case we have to revert the pref for whatever reasons we need test coverage, otherwise we end up in a limbo state where we can't get out of.
Attached patch Patch v2Splinter Review
Attachment #8867091 - Attachment is obsolete: true
Attachment #8868406 - Flags: review?(ckerschb)
Comment on attachment 8868406 [details] [diff] [review]
Patch v2

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

Yoshi, thanks, that looks great. Should we potentially use _data_uri_legacy.* as the postfix for all of those tests? Or are you keeping a list of all of those tests which we can remove eventually. In fact, I think we could also just do a DXR search for |security.data_uri.unique_opaque_origin| and just check where the pref is false, right?
Attachment #8868406 - Flags: review?(ckerschb) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Or are you keeping
> a list of all of those tests which we can remove eventually. In fact, I
> think we could also just do a DXR search for
> |security.data_uri.unique_opaque_origin| and just check where the pref is
> false, right?
Yeah, I think a DXR search is easier so I didn't maintain the list.
Pushed by yhuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22c50e492cd2
rewrite test_bug379959.html. r=ckerschb
https://hg.mozilla.org/mozilla-central/rev/22c50e492cd2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.