Given the recent change in the specification: https://github.com/w3c/webappsec-secure-contexts/issues/42 we should follow suit to remove the risk of breaking websites. We should change our implementation of isSecureContext to our implementation of isSecureContextIfOpenerIgnored and remove the relevant code.

Comment on attachment 8925823 [details] [diff] [review] Always ignore the window opener when calculating secure context Review of attachment 8925823 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/passwordmgr/test/browser/browser_http_autofill.js @@ +22,2 @@ > add_task(async function test_http_autofill() { > for (let scheme of ["http", "https"]) { Did you intend to keep this test commented out? We normally don't land commented-out code.

Comment on attachment 8925823 [details] [diff] [review] Always ignore the window opener when calculating secure context >+++ b/testing/web-platform/meta/secure-contexts/basic-popup-and-iframe-tests.html.ini Please pull in https://github.com/w3c/web-platform-tests/commit/dd83f891b80aa9b05e5fe35d1680e53c6a435b07 instead, since that already exists. If you just land it on our side as part of your changes, the merge with upstream will be a no-op and everything will be happy without us losing test coverage at any point. >+++ b/toolkit/components/passwordmgr/test/browser/browser.ini >+ form_cross_origin_insecure_action.html This file is not in the diff, nor is it used by anything in the patch. Why this change? > +++ b/toolkit/components/passwordmgr/test/browser/browser_http_autofill.js Please either just remove the commented-out code if it's no longer needed or document clearly why it's commented out. The code changes look awesome, but r- for the test issues.

Comment on attachment 8925823 [details] [diff] [review] Always ignore the window opener when calculating secure context Review of attachment 8925823 [details] [diff] [review]: ----------------------------------------------------------------- Took a quick skim and it looks good, but I'll wait to r+ the next version bz asked for.

Update the web platform tests with the changes from https://github.com/w3c/web-platform-tests/commit/dd83f891b80aa9b05e5fe35d1680e53c6a435b07?diff=unified

Fixed the issues from comments 2 and 3. The reason I added form_cross_origin_insecure_action.html to browser.ini is that when I ran the tests locally as a single test, it wasn't copied over, so that portion of the test would fail. I moved it over specifically to under toolkit/components/passwordmgr/test/browser/browser_http_autofill.js so that the dependency is more clear. try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2287184bc2286c34b70efa26ea5dab6e4001a853

Comment on attachment 8928361 [details] [diff] [review] Always ignore the window opener when calculating secure context >+support-files = >+ form_cross_origin_insecure_action.html Ah, thank you for explaining what's going on here. This test (browser_http_autofill.js) also needs form_cross_origin_secure_action.html as a support file, right? Do you mind adding it here as well? r=me

Comment on attachment 8928361 [details] [diff] [review] Always ignore the window opener when calculating secure context Review of attachment 8928361 [details] [diff] [review]: ----------------------------------------------------------------- Thank you very much for cleaning this up Kate!

Comment on attachment 8928360 [details] [diff] [review] Pull over the secure context test changes from Mike West Review of attachment 8928360 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, great stuff!

Comment on attachment 8928361 [details] [diff] [review] Always ignore the window opener when calculating secure context Review of attachment 8928361 [details] [diff] [review]: ----------------------------------------------------------------- r=dveditz

Rebased for nsGlobalWindow{Inner,Outer} changes try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d3e975eede12cd20c516003702d47d06b474acc

Comment on attachment 8930586 [details] [diff] [review] Pull over the secure context test changes from Mike West This already landed by way of bug 1419296.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/681fece780ae Don't consider opener when calculating IsSecureContext. r=bz, r=dveditz

Backed out changeset 681fece780ae (bug 1410364) for failing in /secure-contexts/basic-popup-and-iframe-tests.html r=backout a=backout on a CLOSED TREE https://hg.mozilla.org/integration/mozilla-inbound/rev/52ff139184daf909e342f5940805231ab3659ca9 https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=636b8219055715091580f3719210b6c941423982&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

1) Rebased 2) Remove .ini file for Web Platform Tests https://treeherder.mozilla.org/#/jobs?repo=try&revision=163f63df2ba0eabae218a5ce6921c5936d22f7b1 Will mark as checkin-needed when tests finish

Rebased to current

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6784a27f54ff Don't consider opener when calculating IsSecureContext. r=bz, r=dveditz