Closed Bug 1390770 Opened 7 years ago Closed 7 years ago

rewrite editor/libeditor/tests/test_bug289384.html for new data: URI inheritance model

Categories

(Core :: DOM: Security, enhancement)

Product:
Core
Component:
DOM: Security
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

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

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files, 1 obsolete file)

libeditor_fix.patch
2.41 KB, patch
Details | Diff | Splinter Review
Patch v2.
2.78 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
This test only runs on mac.
Using the latest TRY run from: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37554efc3fb036d426fed8b8aca58148fcb8a4da I also see the following errors for editor tests on OSX: Failed editor/libeditor/tests/test_bug289384.html optOS X 10.10 optOS X 10.10 debug debug Failed editor/libeditor/tests/test_bug318065.html optOS X 10.10 optOS X 10.10 debug debug Failed editor/libeditor/tests/test_bug332636.html optOS X 10.10 optOS X 10.10 debug debug Failed editor/libeditor/tests/test_bug414526.html optOS X 10.10 optOS X 10.10 debug debug Failed editor/libeditor/tests/test_bug417418.html optOS X 10.10 optOS X 10.10 debug debug Running each of the tests (except test_bug289384.html) individually on my local OSX machine doesn't report any failures. Maybe they are all somehow related to test_bug289384.html itself. Probably worth checking.
I also think the other failures are casued by test_bug289384.html, if there's still other failures I'll file another bug.
Comment on attachment 8897803 [details] [diff] [review] Patch. Review of attachment 8897803 [details] [diff] [review]: ----------------------------------------------------------------- Hi Masayuki-san These test contains two data: URIs, and the inner one use 'opener', to make it simpler, I just configured the pref to false. I found you also reviewed other fixs for editor from ckerschb as well, can you help to reivew this? thanks
Attachment #8897803 - Flags: review?(masayuki)
Comment on attachment 8897803 [details] [diff] [review] Patch. I received some requests to review similar patches. But they move content in data URI to separate file(s). Could you explain the reason why this should be change the pref rather than creating another file?
Flags: needinfo?(allstars.chh)
Comment on attachment 8897803 [details] [diff] [review] Patch. > addLoadEvent(function() { >+ SpecialPowers.pushPrefEnv({"set": [ >+ ["security.data_uri.unique_opaque_origin", false]]}, test); >+}); >+ >+function test() { > var win = window.open("data:text/html,<a href=\"data:text/html,<body contenteditable onload='opener.continueTest(window);'>foo bar</body>\">link</a>", "", "test-289384"); If I don't configure the pref, then I have to add two files, var win = window.open("file_289384-1.html"); --file_289384-1.html <a href="file_289384-2.html">link</a> --file_289384-2.html <body contenteditable onload='opener.continueTest(window);'>foo bar </body> However opener will be null now, right? How should I access 'continueTest' here? If you have better suggestions I'll follow it. Thanks
Flags: needinfo?(allstars.chh)
My question is, why you don't separate the file, but instead, you change the pref from the default settings? I think that unless the test does check the behavior when "security.data_uri.unique_opaque_origin" is set to false, it check the behavior with default settings and use separated files.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #7) > My question is, why you don't separate the file, but instead, you change the > pref from the default settings? > > I think that unless the test does check the behavior when > "security.data_uri.unique_opaque_origin" is set to false, it check the > behavior with default settings and use separated files. I agree with Masayuki. We shouldn't explicitly flip the pref for the test. Why not just loading two extra files? Seems to work locally.
Flags: needinfo?(allstars.chh)
Attached patch Patch v2. — — Splinter Review
it turned out I can access opener. so update the test by adding files.
Attachment #8897803 - Attachment is obsolete: true
Attachment #8897803 - Flags: review?(masayuki)
Flags: needinfo?(allstars.chh)
Attachment #8898247 - Flags: review?(masayuki)
(In reply to Yoshi Huang [:allstars.chh] from comment #9) > it turned out I can access opener. > so update the test by adding files. Awesome - thanks!
Comment on attachment 8898247 [details] [diff] [review] Patch v2. I assume that you confirmed this on tryserver.
Attachment #8898247 - Flags: review?(masayuki) → review+
Pushed by yhuang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c309051872fa rewrite test_bug289384.html for new data: URI inheritance model. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/c309051872fa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: