Closed Bug 1600326 Opened 6 years ago Closed 6 years ago

Several tests unexpected fail/pass after enabling the prefs for COOP, COEP, CORP

Categories

(Core :: DOM: Networking, defect, P1)

Product:
Core
Component:
DOM: Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla73
Tracking Flags:
Tracking Status
firefox73 --- fixed

People

(Reporter: tt, Assigned: CuveeHsu)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(3 files)

Bug 1600326 - P1 avoid wrongly COOP error for subframe, r=ttung
47 bytes, text/x-phabricator-request
Details | Review
Bug 1600326 - P2 avoid propagating error if we don't have COOP without window global, r=ttung
47 bytes, text/x-phabricator-request
Details | Review
Bug 1600326 - P3 use lazy pref getter in MixedContentBlocker, r=bzbarsky
47 bytes, text/x-phabricator-request
Details | Review

See https://treeherder.mozilla.org/#/jobs?repo=try&revision=7389149789863e86855ddbad42bb887fa05730bf&selectedJob=278791030

I suspect these unexpected results because of no process switching. If you find it's not, please feel free to open another bug to track those unrelated but failed tests.

For instance:
toolkit/components/remotebrowserutils/tests/browser/browser_httpResponseProcessSelection.js
toolkit/components/remotebrowserutils/tests/browser/browser_oopProcessSwap.js

Might be related:
browser/base/content/test/tabs/browser_new_web_tab_in_file_process_pref.js
toolkit/components/antitracking/test/browser/browser_doublyNestedTracker.js (TEST-UNEXPECTED-PASS)
dom/notification/test/mochitest/test_notification_tag.html (TEST-UNEXPECTED-PASS)

The try hasn't finished yet, but I suspect the some of the existing unexpected test failures related to no process switching after enabling the prefs.

Flags: needinfo?(valentin.gosu)
Flags: needinfo?(juhsu)
Priority: -- → P1
See Also: → 1598663

(In reply to Tom Tung [:tt, :ttung] from comment #1)

The try hasn't finished yet, but I suspect the some of the existing unexpected test failures related to no process switching after enabling the prefs.

Yes, please check Bug 1593439 and Bug 1595284 for the list I fixed.
It's pretty much likely the same list.

Flags: needinfo?(juhsu)

test failures after eanbling the prefs related to SAB:

TEST-UNEXPECTED-FAIL | toolkit/components/remotebrowserutils/tests/browser/browser_httpResponseProcessSelection.js | no process switch - Got web, expected file
TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_new_web_tab_in_file_process_pref.js | Check that new http tab opened from file loaded in file content process. - 4457 == 4628 -
TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/browser_download_canceled.js | Uncaught exception - TypeError: content.wrappedJSObject.streamClosed is undefined
TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - TypeError: window.opener is null at my_ok@http://mochi.test:8888/tests/dom/serviceworkers/test/fetch/index.html:19:5
TEST-UNEXPECTED-PASS | security/manager/ssl/tests/mochitest/mixedcontent/test_unsecureIframeRedirect.html | fail-if condition in manifest - We expected at least one failure
TEST-UNEXPECTED-PASS | browser/base/content/test/siteIdentity/browser_bug1045809.js | We expect at least one assertion to fail because this test file is marked as fail-if in the manifest. -
TEST-UNEXPECTED-PASS | browser/base/content/test/siteIdentity/browser_mixedContentFramesOnHttp.js | We expect at least one assertion to fail because this test file is marked as fail-if in the manifest. -
TEST-UNEXPECTED-FAIL | accessible/tests/browser/bounds/browser_test_resolution.js | Remote IFRAME is in a different process. - Got true, expected false
TEST-UNEXPECTED-FAIL | accessible/tests/browser/e10s/browser_caching_description.js | Remote IFRAME is in a different process. - Got true, expected false
TEST-UNEXPECTED-FAIL | accessible/tests/browser/e10s/browser_events_show.js | Remote IFRAME is in a different process. - Got true, expected false
TEST-UNEXPECTED-FAIL | browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js | [JavaScript Error: "NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIProcessSwitchRequestor.crossOriginOpenerPolicy]" {file: "resource:///modules/sessionstore/SessionStore.jsm" line: 2714}] -
TEST-UNEXPECTED-FAIL | dom/tests/browser/browser_windowProxy_transplant.js | Missing expected exception. Should get a remote outer window proxy when accessing old window proxy -
TEST-UNEXPECTED-FAIL | accessible/tests/browser/fission/browser_content_tree.js | Remote IFRAME is in a different process. - Got true, expected false
TEST-UNEXPECTED-FAIL | accessible/tests/browser/fission/browser_hidden_iframe.js | IFRAME accessible should have a single child - Got 0, expected 1
TEST-UNEXPECTED-FAIL | accessible/tests/browser/fission/browser_src_change.js | Remote IFRAME is in a different process. - Got true, expected false
TEST-UNEXPECTED-PASS | browser/base/content/test/siteProtections/browser_protections_UI.js | We expect at least one assertion to fail because this test file is marked as fail-if in the manifest. -
TEST-UNEXPECTED-PASS | toolkit/components/antitracking/test/browser/browser_doublyNestedTracker.js | We expect at least one assertion to fail because this test file is marked as fail-if in the manifest. -
TEST-UNEXPECTED-PASS | devtools/client/inspector/test/browser_inspector_search-04.js | We expect at least one assertion to fail because this test file is marked as fail-if in the manifest. -
TEST-UNEXPECTED-PASS | devtools/client/inspector/test/browser_inspector_search-05.js | We expect at least one assertion to fail because this test file is marked as fail-if in the manifest. -
TEST-UNEXPECTED-PASS | devtools/server/tests/browser/browser_animation_getSubTreeAnimations.js | We expect at least one assertion to fail because this test file is marked as fail-if in the manifest. -
TEST-UNEXPECTED-PASS | devtools/server/tests/browser/browser_animation_playPauseIframe.js | We expect at least one assertion to fail because this test file is marked as fail-if in the manifest. -
TEST-UNEXPECTED-PASS | devtools/server/tests/browser/browser_storage_dynamic_windows.js | We expect at least one assertion to fail because this test file is marked as fail-if in the manifest. -
TEST-UNEXPECTED-PASS | dom/base/test/test_window_cross_origin_props.html | fail-if condition in manifest - We expected at least one failure
TEST-UNEXPECTED-FAIL | js/xpconnect/tests/mochitest/test_isRemoteProxy.html | frames[1] is a remote proxy if Fission is enabled - got false, expected true
TEST-UNEXPECTED-FAIL | js/xpconnect/tests/mochitest/test_isRemoteProxy.html | frames[1].location is a remote proxy if Fission is enabled - got false, expected true

Assignee: nobody → juhsu

The latest try (I pushed in today's morning; should include the fix in bug 1592894)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c872348350960bbbb20f4ac7407077dcda985aed&selectedJob=281028690

The remaining issues seem to be related to process switching, many Fission mochitests unexpected passed or failed.

For instance:
TEST-UNEXPECTED-FAIL | js/xpconnect/tests/mochitest/test_isRemoteProxy.html | frames[1] is a remote proxy if Fission is enabled - got false, expected true
TEST-UNEXPECTED-FAIL | dom/base/test/browser_aboutnewtab_process_selection.js | Uncaught exception - undefined - timed out after 50 tries.
TEST-UNEXPECTED-FAIL | accessible/tests/browser/e10s/browser_caching_description.js | Remote IFRAME is in a different process. - Got true, expected false

Matt, maybe you have more insight into the reason why?

There only a few mochitests failed (they are probably also related to process switching) left and I list them below:
accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js
toolkit/components/remotebrowserutils/tests/browser/browser_oopProcessSwap.js
dom/tests/browser/browser_windowProxy_transplant.js | Missing expected exception. Should get a remote outer window proxy when accessing old window proxy -

One more clue I can provide is that is I turn the pref for COOP off and the try is green again, so there must be something related to that.

Flags: needinfo?(matt.woodrow)

(In reply to Tom Tung [:tt, :ttung] from comment #4)
Looking into "accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js" and I found:

0:54.36 GECKO(38762) [Child 38764, Main Thread] WARNING: Trying to request nsIHttpChannel from DocumentChannelChild, this is likely broken: file /Users/tomtung/Work/mozilla-central/netwerk/ipc/DocumentChannelChild.cpp, line 63

1:04.31 GECKO(38762) JavaScript error: resource:///modules/sessionstore/SessionStore.jsm, line 2708: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIProcessSwitchRequestor.crossOriginOpenerPolicy] 1:04.31 GECKO(38762) [Parent 38762, Main Thread] WARNING: we only accept nsIURI interface type, patch welcome: file /Users/tomtung/Work/mozilla-central/dom/ipc/PropertyBagUtils.cpp, line 112 1:04.31 GECKO(38762) [Child 38768, Main Thread] WARNING: Trying to request nsIHttpChannel from DocumentChannelChild, this is likely broken: file /Users/tomtung/Work/mozilla-central/netwerk/ipc/DocumentChannelChild.cpp, line 63 1:04.35 INFO Console message: [JavaScript Error: "NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIProcessSwitchRequestor.crossOriginOpenerPolicy]" {file: "resource:///modules/sessionstore/SessionStore.jsm" line: 2708}]

So, these tests failed at switchRequestor.crossOriginOpenerPolicy. Since the the code seems to expect to be called before OnStartRequest(), I suspect there is a timing issue (Somehow OnStartRequest is called after the onMayChangeProcess)?

Below fix the failure on "accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js", but I believe this is definitely not the correct way to fix it. (That just a workaround to verify the code causes the test to fail)

-    if (
-      E10SUtils.useCrossOriginOpenerPolicy() &&
-      switchRequestor.crossOriginOpenerPolicy ==
-        Ci.nsILoadInfo.OPENER_POLICY_SAME_ORIGIN_EMBEDDER_POLICY_REQUIRE_CORP
-    ) {
-      // We want documents with a SAME_ORIGIN_EMBEDDER_POLICY_REQUIRE_CORP
-      // COOP policy to be loaded in a separate process for which we can enable
-      // high resolution timers.
-      preferredRemoteType =
-        E10SUtils.WEB_REMOTE_COOP_COEP_TYPE_PREFIX + resultPrincipal.siteOrigin;
-    } else if (isCOOPSwitch) {
-      // If it is a coop switch, but doesn't have this flag, we want to switch
-      // to a default remoteType
-      preferredRemoteType = E10SUtils.DEFAULT_REMOTE_TYPE;
-    }
-    debug(
-      `[process-switch]: currentRemoteType (${currentRemoteType}) preferredRemoteType: ${preferredRemoteType}`
-    );
+    try {
+     let coop = switchRequestor.crossOriginOpenerPolicy;
+      if (
+        E10SUtils.useCrossOriginOpenerPolicy() &&
+        coop ==
+          Ci.nsILoadInfo.OPENER_POLICY_SAME_ORIGIN_EMBEDDER_POLICY_REQUIRE_CORP
+      ) {
+        // We want documents with a SAME_ORIGIN_EMBEDDER_POLICY_REQUIRE_CORP
+        // COOP policy to be loaded in a separate process for which we can enable
+        // high resolution timers.
+        preferredRemoteType =
+          E10SUtils.WEB_REMOTE_COOP_COEP_TYPE_PREFIX + resultPrincipal.siteOrigin;
+      } else if (isCOOPSwitch) {
+        // If it is a coop switch, but doesn't have this flag, we want to switch
+        // to a default remoteType
+        preferredRemoteType = E10SUtils.DEFAULT_REMOTE_TYPE;
+      }
+    } catch (e) {
+      debug(
+        `[process-switch]: currentRemoteType (${currentRemoteType}) preferredRemoteType: ${preferredRemoteType}`
+      );
+    }

I believe the correct fix should somehow ensure the order for OnStartRequest and onMayChangeProcess? I've seen such warning appears in most of failures, so I believe this is one of the major causes for failures. Junior, I'm not familiar with the code around there, maybe you can take a look? Thanks!

Flags: needinfo?(juhsu)

I don't think onMayChangeProcess happens before OnStartRequest (it's called from DocumentLoadListener::OnStartRequest).

I think we can call OnStartRequest on our listeners without running ComputeCrossOriginOpenerPolicyMismatch though, particularly if the channel gets aborted/cancelled.

Your patch seems basically fine, except that it's not running the isCOOPSwitch branch when it probably should.

Why don't we just initialize mComputedCrossOriginOpenerPolicy = Some(nsILoadInfo::OPENER_POLICY_NULL); in the nsHttpChannel constructor?

Flags: needinfo?(matt.woodrow)

I'm with Matt for the patch.
Maybe Bug 1598663 helps to ease the tests?

Tom, I'm assigning this to you since you're already working on this.
Feel free to bounce back.

Assignee: juhsu → ttung
Flags: needinfo?(juhsu)

Assign back to Junior since I feel like I lack the knowledge on channels stuff for fixing this properly and I'm not sure if I have enough time to fix all the remaining issues before the end of this year. (For instance, there are still a few failures after applying the temporary fix with handling the isCOOPSwitch [1] and they all look like wrong process issue).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=124dcfc4203c297db69698ba260a563b56975b98&selectedJob=281528596

(In reply to Matt Woodrow (:mattwoodrow) from comment #7)

I don't think onMayChangeProcess happens before OnStartRequest (it's called from DocumentLoadListener::OnStartRequest).

I think we can call OnStartRequest on our listeners without running ComputeCrossOriginOpenerPolicyMismatch though, particularly if the channel gets aborted/cancelled.

I see. Thanks! That probably means mComputedCrossOriginOpenerPolicy needs to be handled better than now (because it seems that the assumption for that code is not correct?)

Your patch seems basically fine, except that it's not running the isCOOPSwitch branch when it probably should.

Why don't we just initialize mComputedCrossOriginOpenerPolicy = Some(nsILoadInfo::OPENER_POLICY_NULL); in the nsHttpChannel constructor?

I guess I probably misunderstand what you mean. Does that mean we are changing mComputedCrossOriginOpenerPolicy from Maybe<XXX> to XXX and remove the check innsHttpChannel::GetCrossOriginOpenerPolicy? I don't think it's the right way to do because it seems to violate the reason for adding the check

(In reply to Junior [:junior] from comment #8)

I'm with Matt for the patch.
Maybe Bug 1598663 helps to ease the tests?

Note that the base for the try has already included the fix in Bug 1598663

Assignee: ttung → juhsu

(In reply to Tom Tung [:tt, :ttung] from comment #9)
Had a meeting with Junior, so we should fix the current issue (failures on accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js [Fission enabled]) in some ways.

And then look into remaining issues. There are many failures on Fission mochitests, but just two non-Fission test failures.
They are:

  • browser/base/content/test/trackingUI/browser_trackingUI_pbmode_exceptions.js | Test timed out -
  • TEST-UNEXPECTED-FAIL | dom/indexedDB/test/test_objectCursors.html | Test timed out.
  • TEST-UNEXPECTED-FAIL | dom/indexedDB/test/test_objectStore_getAllKeys.html | Test timed out.
  • TEST-UNEXPECTED-FAIL | dom/indexedDB/test/test_objectStore_inline_autoincrement_key_added_on_put.html | Test timed out..

For the Fission mochitests failures, most of them failed because window not found (I suspect the root cause is something related to process switching)

(In reply to Tom Tung [:tt, :ttung] from comment #6)

Below fix the failure on "accessible/tests/browser/states/browser_deck_has_out_of_process_iframe.js", but I believe this is definitely not the correct way to fix it. (That just a workaround to verify the code causes the test to fail)

Here's the reason:
Looks like we will check if we need to switch process for every document load.
https://searchfox.org/mozilla-central/rev/c61720a7d0c094d772059f9d6a7844eb7619f107/netwerk/ipc/DocumentLoadListener.cpp#720-727

However, COOP only happens on top-level document, we won't set COOP for subframe.
I believe what valentin wanted to check is we get COOP after OnStartRequest is called, so I'm fine with matt's suggestion.
We could return NS_ERROR_UNAVAILABLE if !mOnStartRequestCalled instead of the workaround.

Looks like window-failure.https.html with fission is the only one we left.

(In reply to Junior [:junior] from comment #16)

Looks like window-failure.https.html with fission is the only one we left.

I printed some log on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12506801406c08845a56362cd25610785301df83&selectedJob=281719406

It seems the coop for the document is NULL (not set) while serializing. Thus, it throws while it's postMessage'ing. That indicates there is a timing issue?

(In reply to Tom Tung [:tt, :ttung] from comment #17)

It seems the coop for the document is NULL (not set) while serializing. Thus, it throws while it's postMessage'ing. That indicates there is a timing issue?

Maybe it's because BrowsingContext::PostMessageMoz is called (I haven't handled the CrossOriginIsolaed check there yet). I'll check that on try (because I cannot reproduce that locally)

(In reply to Tom Tung [:tt, :ttung] from comment #18)

(In reply to Tom Tung [:tt, :ttung] from comment #17)

It seems the coop for the document is NULL (not set) while serializing. Thus, it throws while it's postMessage'ing. That indicates there is a timing issue?

Maybe it's because BrowsingContext::PostMessageMoz is called (I haven't handled the CrossOriginIsolaed check there yet). I'll check that on try (because I cannot reproduce that locally)

It's what I suspected. I will fix that in D57641 (bug 1594748)

Look like browser/base/content/test/performance/browser_preferences_usage.js is the last one?
And maybe that will be fixed by the newer version of P3?

(In reply to Tom Tung [:tt, :ttung] from comment #20)

Look like browser/base/content/test/performance/browser_preferences_usage.js is the last one?
And maybe that will be fixed by the newer version of P3?

Yes, it fixes browser_preferences_usage.js
I'll ask for another review after I fix another treeherder orange.(In reply to Tom Tung [:tt, :ttung] from comment #19)

It's what I suspected. I will fix that in D57641 (bug 1594748)

Cool. Looks like we have everything!

Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-triaged]
Attachment #9116591 - Attachment description: Bug 1600326 - P3 use var cache in MixedContentBlocker, r=ckerschb → Bug 1600326 - P3 use lazy pref getter in MixedContentBlocker, r=ckerschb

On the latest try I pushed, I still got:

TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_preferences_usage.js | dom.securecontext.whitelist should not be accessed more than 40 times. - 52 <= 40 - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/performance/browser_preferences_usage.js :: checkPrefGetters :: line 42

Other than that, it looks good.

(In reply to Tom Tung [:tt, :ttung] (OOO from 2019/12/23 ~ 2020/01/14) from comment #22)

On the latest try I pushed, I still got:

TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_preferences_usage.js | dom.securecontext.whitelist should not be accessed more than 40 times. - 52 <= 40 - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/performance/browser_preferences_usage.js :: checkPrefGetters :: line 42

Other than that, it looks good.

Fixed with the update.

Attachment #9116591 - Attachment description: Bug 1600326 - P3 use lazy pref getter in MixedContentBlocker, r=ckerschb → Bug 1600326 - P3 use lazy pref getter in MixedContentBlocker, r=bzbarsky
Pushed by juhsu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/acd569b6f0b9 P1 avoid wrongly COOP error for subframe, r=ttung https://hg.mozilla.org/integration/autoland/rev/6d35f3f95f31 P2 avoid propagating error if we don't have COOP without window global, r=ttung https://hg.mozilla.org/integration/autoland/rev/42a4d256fbaa P3 use lazy pref getter in MixedContentBlocker, r=bzbarsky
Regressions: 1606427

https://hg.mozilla.org/mozilla-central/rev/acd569b6f0b9
https://hg.mozilla.org/mozilla-central/rev/6d35f3f95f31
https://hg.mozilla.org/mozilla-central/rev/42a4d256fbaa

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox73: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
Regressions: 1707469
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: