1337271 - Convertparser/htmlparser/tests/mochitest/test_html5_tree_construction* to comply with new data: URI inheritance model

Status: RESOLVED FIXED

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

Description

[Christoph Kerschbaumer [:ckerschb]]

Flip pref [1] to 'false' and update tests within parser/ to still work with new data: URI behavior.

[1] pref ("security.data_uri.inherit_security_context", true);

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1337271_convert_parser_tests.patch

Smaug, within this patch I am updating 3 tests:
* test_html5_tree_construction.html
* test_html5_tree_construction_part2.html
Both of these tests rely on parser_web_testrunner.js which creates a data: URI here [1] which is (when the pref is flipped) cross origin.

* parser/htmlparser/tests/mochitest/test_compatmode.html
I tried to keep as much as possible untouched within that test. Please note that 'expected' was unused within insert_iframe, hence I removed it. Further I think it makes sense to just report the frameId to the parent and then use SpecialPowers to reach into the iframe to check for .compatMode.

[1] https://dxr.mozilla.org/mozilla-central/source/parser/htmlparser/tests/mochitest/parser_web_testrunner.js#116

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8885415 [details] [diff] [review]
bug_1337271_convert_parser_tests.patch

rs+

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Ah, it seems that using SpecialPowers causes some troubles:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9208f83cb62721da41edcd0f45255614f226957

Have to investigate ...

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1337271_convert_parser_tests.patch

Hey William (it seems Henri is out for a while, right?) we are updating the way the data: URI inheritance security model works within Firefox. By flipping the pref security.data_uri.unique_opaque_origin data: URIs become cross origin with the including context. We identified 3 tests within parser/ that are failing when we flip that pref:
(a) test_compatmode.html
(b) test_html5_tree_construction.html
(c) test_html5_tree_construction_part2.html

While I was able to resolve (a) by using a postmessage handler, I am facing quite some difficulties with (b) and (c). Both of these tests rely on parser_web_testrunner.js. Initially I rewrote
> - var domAsString = docToTestOutput(e.target.contentDocument);
> + var domAsString = docToTestOutput(SpecialPowers.wrap(e).target.contentDocument);

which in theory should work, right? But many tests are failing. I tried to debug for a little while, but no result. I assume that SpecialPowers can't forward all the calls correctly, but that's only my suspicion.

Anyway, in my second attempt (see attached patch) I am using frame.srcdoc instead of frame.src which makes (b) work, but tests within (c) are still failing. I was wondering if you have any other suggestions how we should/could rewrite that test to make it work when the pref is flipped. Thanks for your help!

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Please note that I landed changes for test_compatmode.html over in Bug 1384500, so this test now solely is about:
* test_html5_tree_construction.html
* test_html5_tree_construction_part2.html

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1337271_convert_parser_tests.patch

Henri and/or William. We are updating the data: URI inheritance model. In the new world, data: URIs loaded within an iframe become cross origin. When flipping the pref security.data_uri.unique_opaque_origin the following two tests within parser/ fail:
1: test_html5_tree_construction.html
2: test_html5_tree_construction_part2.html

Instead of using a data: URI to load individual tests within parser_web_testrunner.js, I updated the code to rely on srcdoc instead. Interestingly, all 726 within  test_html5_tree_construction.html, but two tests within test_html5_tree_construction_part2.html fail (all other 826 tests pass). I couldn't figure out why as of now (See detailed errors underneath).

To make debugging easier I separated the two failing subtests from the regular testsuite and load them through failing_tests.dat.

Do any of you have an idea why those tests might be failing? If yes, how we should update them? Or otherwise how I can debug to figure out the exact problem? Thanks for your help!

%--------snip----------------

Unexpected Failure:
Matched: false
Input: 
<p><table></p>Expected:
| <html>
|   <head>
|   <body>
|     <p>
|       <p>
|       <table>Output:
| <html>
|   <head>
|   <body>
|     <p>
|     <p>
|     <table>
-

Unexpected Failure:
Matched: false
Input: 
<p><table></table>Expected:
| <html>
|   <head>
|   <body>
|     <p>
|       <table>Output:
| <html>
|   <head>
|   <body>
|     <p>
|     <table>
-

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

It seems Comment 6 wasn't very precise because I accidentally left out a word :-(. But to be precise, only two tests are failing in total:

                                           PASS | FAIL
test_html5_tree_construction.html           726 |    0
test_html5_tree_construction_part2.html     826 |    2

Comment 8

[Henri Sivonen (:hsivonen)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Created attachment 8892426 [details] [diff] [review]
> bug_1337271_convert_parser_tests.patch
> 
> Henri and/or William. We are updating the data: URI inheritance model. In
> the new world, data: URIs loaded within an iframe become cross origin. When
> flipping the pref security.data_uri.unique_opaque_origin the following two
> tests within parser/ fail:
> 1: test_html5_tree_construction.html
> 2: test_html5_tree_construction_part2.html
> 
> Instead of using a data: URI to load individual tests within
> parser_web_testrunner.js, I updated the code to rely on srcdoc instead.
> Interestingly, all 726 within  test_html5_tree_construction.html, but two
> tests within test_html5_tree_construction_part2.html fail (all other 826
> tests pass). I couldn't figure out why as of now (See detailed errors
> underneath).
> 
> To make debugging easier I separated the two failing subtests from the
> regular testsuite and load them through failing_tests.dat.
> 
> Do any of you have an idea why those tests might be failing? If yes, how we
> should update them? Or otherwise how I can debug to figure out the exact
> problem? Thanks for your help!
> 
> %--------snip----------------
> 
> Unexpected Failure:
> Matched: false
> Input: 
> <p><table></p>Expected:
> | <html>
> |   <head>
> |   <body>
> |     <p>
> |       <p>
> |       <table>Output:
> | <html>
> |   <head>
> |   <body>
> |     <p>
> |     <p>
> |     <table>
> -
> 
> Unexpected Failure:
> Matched: false
> Input: 
> <p><table></table>Expected:
> | <html>
> |   <head>
> |   <body>
> |     <p>
> |       <table>Output:
> | <html>
> |   <head>
> |   <body>
> |     <p>
> |     <table>
> -

These look like the tests expect quirks-mode parsing, but for whatever reason, the tests get parsed in the standards mode (as if the input included a standards mode-triggering doctype).

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Instead of using a data: URI to load individual tests within
> parser_web_testrunner.js, I updated the code to rely on srcdoc instead.

srcdoc forces the parser to use no-quirks parsing, so that's why you see the tests that test the one quirk fail. (There indeed is only one parsing quirk: https://hsivonen.fi/last-html-quirk/ )

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Henri Sivonen (:hsivonen) from comment #9)
> srcdoc forces the parser to use no-quirks parsing, so that's why you see the
> tests that test the one quirk fail. (There indeed is only one parsing quirk:
> https://hsivonen.fi/last-html-quirk/ )

Thanks Henri. Would you be fine with me just updating those two tests to make them work with srcdoc?

Comment 11

[Henri Sivonen (:hsivonen)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> (In reply to Henri Sivonen (:hsivonen) from comment #9)
> > srcdoc forces the parser to use no-quirks parsing, so that's why you see the
> > tests that test the one quirk fail. (There indeed is only one parsing quirk:
> > https://hsivonen.fi/last-html-quirk/ )
> 
> Thanks Henri. Would you be fine with me just updating those two tests to
> make them work with srcdoc?

No, because a sync of test cases from upstream would overwrite your changes. Please add them to the list of known failures instead (with a comment why): https://searchfox.org/mozilla-central/source/parser/htmlparser/tests/mochitest/html5_tree_construction_exceptions.js

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1337271_update_parser_tests.patch

Henri, thanks for your help. I marked the two tests as failing which now show up as 'todo' when running the tests.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca187b5c23eb
Convert tests within parser/ to comply with new data: URI inheritance model. r=hsivonen

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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