Closed Bug 1397653 Opened 2 years ago Closed 2 years ago

Update tests within docshell/ to comply with new toplevel data: URI navigation policy

Categories

(Core :: DOM: Security, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

No description provided.
Blocks: 1380959
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Smaug, overall we are facing several failing tests within docshell/ when flipping the pref |security.data_uri.block_toplevel_data_uri_navigations|:
* docshell/test/navigation/test_bug13871.html
* docshell/test/navigation/test_bug270414.html
* docshell/test/navigation/test_bug278916.html
* docshell/test/navigation/test_bug279495.html
* docshell/test/navigation/test_bug430624.html
* docshell/test/navigation/test_not-opener.html
* docshell/test/navigation/test_not-opener.html
* docshell/test/navigation/test_sessionhistory.html
* docshell/test/navigation/test_triggeringprincipal_window_open.html
* docshell/test/test_bug598895.html
* docshell/test/test_bug637644.html
* docshell/base/crashtests/914521.html

Most of the failing tests within docshell/test/navigation rely on NavigationUtils.js. Now that we are loading the data: URI from an external file .textContent as well as .innerHTML include the \n, hence I use .trim() before performing the string comparision.

The test file_scrollRestoration.html is not really relying on the data: URI itself, hence I suppose it's fine to just use about:blank instead of using an external file.

The test test_triggeringprincipal_window_open.html is using window.open("data:", since that can not happen anymore, I suppose it's fine to remove that test. If you prefer, I am also happy to use setPushPrefEnv and set the pref to false for that test.

For thest test crashtests/914521.html, we already set the pref security.data_uri.unique_opaque_origin, hence I think it makes sense to also explicity set the pref for this test.

The test chrome/test_bug364461.xul uses several data: URIs, I suppose it's fine to keep the test as is and just push the pref explicitly for the test, if you prefer, I am happy to load those data: URIs from an external file.

Everything else should be self explanatory, if not, please let me know!

Seems there are no more docshell tests that need to be rewritten:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb517e30d8209331f929f9a3f9fd2b61a5fed63c
Attachment #8906370 - Flags: review?(bugs)
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: P3 → P2
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
Comment on attachment 8906370 [details] [diff] [review]
bug_1397653_docshell_tests.patch

Hmm, Why is using about:blank in file_scrollRestoration.html ok?
http://searchfox.org/mozilla-central/rev/70cfd6ceecacbe779456654b596bbee4f2b8890b/docshell/base/nsDocShell.cpp#12461,12481

You end up changing what is being tested.
Attachment #8906370 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #2)
> You end up changing what is being tested.

Or explain why that is not happening.
(In reply to Olli Pettay [:smaug] from comment #3)
> Or explain why that is not happening.

When looking at the test I thought it just needs some page to navigate away from the current. In my opinion it doesn't really matter that page that is. Then using history.back() should restore the setting that were used on that previous page. Isn't that what the test is doing? If not, then I am happy to rewrite those bits to load some external foo page.
Flags: needinfo?(bugs)
Comment on attachment 8906370 [details] [diff] [review]
bug_1397653_docshell_tests.patch

ok
Flags: needinfo?(bugs)
Attachment #8906370 - Flags: review- → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b64aee9198d
Update tests within docshell/ to comply with new toplevel data: URI navigation policy. r=smaug
https://hg.mozilla.org/mozilla-central/rev/2b64aee9198d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.