Open Bug 1746524 Opened 3 years ago Updated 1 year ago

Enable browser.tabs.documentchannel.parent-controlled pref

Categories

(Core :: DOM: Navigation, task, P1)

task

Tracking

()

REOPENED

People

(Reporter: annyG, Assigned: mccr8)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

(Keywords: perf-alert)

Attachments

(1 file)

Unfortunately, work in bug 1721217 expanded because of a few test failures that came up recently. To make this work more manageable, It makes more sense to land patches in that bug without this pref enabled, and instead track the required work for doing that here.

Blocks: 1743636
No longer blocks: 1743636
Depends on: 1743636
Blocks: 1647550
Type: defect → task
Priority: -- → P1

Anny, should this P1 bug be assigned to you? Or will someone else finish the work to enable the browser.tabs.documentchannel.parent-controlled pref?

Severity: -- → N/A
Flags: needinfo?(agakhokidze)
Flags: needinfo?(agakhokidze)
Depends on: 1742886
Depends on: 1748327
Depends on: 1748364

Just pasting a comment from here, when we enable browser.tabs.documentchannel.parent-controlled and SHIP by default, we will not need to check for isNavigating here anymore.

This tiny task is probably blocked on removal of support for non-SHIP and non-Fission configurations.

Assignee: nobody → continuation
Blocks: 1743636
No longer depends on: 1743636
Depends on: 1765652
Depends on: 1766305
No longer blocks: 1743636
Depends on: 1743636
Depends on: 1776080

I still want to wait for bug 1766305 to merge to m-c, but things seem reasonably green so far on Linux, and it doesn't look like it is breaking anything on Android.

One dodgy thing remaining is that download tests need to be written in a special way, but I'm not sure there's any way around it. I guess I'll write an email to Firefox-dev about that.

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5dda3f9ee3d5 Enable browser.tabs.documentchannel.parent-controlled pref. r=nika

Backed out for causing multiple failures.

Backout link
Push with failures

Link to bc failure log
Link to X5 failure log
Failure line :
TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_tabs_discard_reversed.js
TEST-UNEXPECTED-FAIL | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_userScripts_register.js

Flags: needinfo?(continuation)
Flags: needinfo?(continuation)

I managed to reproduce the X5 failure on my try push. It is an intermittent failure. It seems to fail about half of the time across 9 tries on inbound and 3 out of 8 times on try. I found evidence of a few similar looking intermittent failures in bugzilla, but if it occurs without parent controlled navigation, the failure rate is much lower, because it happened 0 times in a dozen retries.

The BC failure happened 3 out of 3 times on try. It seems odd that it is opt only. Maybe there's a race condition.

Depends on: 1780488

I've fixed the XPCShell failure in bug 1780488. I haven't managed to reproduce the BC failure (browser_ext_tabs_discard_reversed.js), though it does look like an existing issue. This test has already been disabled on OSX, Windows 10 Fission debug, and Linux 64 debug, so I think it is a very flaky test. Maybe I just got unlucky with my landing, though the prior analysis of this test does sound like something this patch could cause to fail more.

With my patch, I tried retriggering the configuration that hit the BC failure in comment 7, and didn't hit the issue after 12 retries. I also re-enabled the test on Linux debug (the debug failure rate was 4 or so times higher before it was disabled) and retriggered it on try 10 times and did not hit the issue.

I think I'm going to try relanding it, after confirming there are no new regressions with my patch. If browser_ext_tabs_discard_reversed.js starts to fail too frequently I'll try talking to the WebExtensions people about disabling the title checking part of the test, as it sounds like it is somewhat racy.

I also tried running browser_ext_tabs_discard_reversed.js locally on OSX debug with --verify and my patch, four or five times, and was unable to reproduce any failure.

The try run looks okay-ish so I'll try relanding this.

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/44255b7d9b1c Enable browser.tabs.documentchannel.parent-controlled pref. r=nika
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch

Backed out for causing xpcshell failures on test_ext_userScripts_register.js (requested by dev)

Backout link

Push with failures

Failure log

Flags: needinfo?(continuation)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

The test only seems to fail with Fission disabled (including on Android), so I'm going to try disabling it under those conditions in 1763197

Depends on: 1763197
Flags: needinfo?(continuation)
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/24d6d6816f34 Enable browser.tabs.documentchannel.parent-controlled pref. r=nika
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Regressions: 1785290
Status: RESOLVED → REOPENED
Flags: needinfo?(continuation)
Resolution: FIXED → ---
Target Milestone: 105 Branch → ---

Thanks.

Flags: needinfo?(continuation)

(In reply to Norisz Fay [:noriszfay] from comment #14)

Backed out for causing xpcshell failures on test_ext_userScripts_register.js (requested by dev)

Backout link

Push with failures

Failure log

== Change summary for alert #35188 (as of Mon, 22 Aug 2022 08:30:10 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
10% bing FirstVisualChange android-hw-a51-11-0-aarch64-shippable-qr warm webrender 374.08 -> 336.42

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=35188

Depends on: 1814293

I did a fresh Linux debug try push to see if any new tests have failed. I filed a bug 1814293 for one that seems to be permafail. Nothing else seems to be permafail, though I am suspicious of the unexpected pass in /navigation-timing/secure-connection-start-non-zero.https.html that happened 2 out of 4 times on my try push, after only happening twice in the last 30 days otherwise on Treeherder.

I'm also suspicious of it because of the "navigation timing" in the path sounds like the sort of thing that a change in navigation could affect.

Depends on: 1814311

I did another Linux debug try run. WPT2, dt8, M7(http3) have a high failure rate, but they are having a very high failure rate on trunk, so I don't think that's a regression. M-a11y-checks dt6 has a high failure rate, but we don't seem to run that at all on m-c or autoland so I'll assume that's not a regression either. The other failures didn't show up on another couple of retriggers.

Yet another Linux debug try run, and it looks like the high failure rates have gone. There are some other failures, have the failing tests moved to other chunks. Doesn't look like it at first glance.

Does this mean that we should try to land again after soft freeze is over?

Flags: needinfo?(continuation)

(In reply to Andreas Farre [:farre] from comment #25)

Does this mean that we should try to land again after soft freeze is over?

The issue that got it backed out initially was a bunch of performance regressions (bug 1785290). I investigated that for awhile but didn't solve the issue. It was a bit odd because the tests that regressed were initiating the loads in the parent (loads were basically started like you were pasting the URL in the URL bar and hitting enter).

Flags: needinfo?(continuation)

The perf problems seems to still be present. The revisions used where base and new. I've tried looking at profiles a bit, but I'm not really seeing anything.

When I looked at it, I think the "new" kind of looked like the old one, shifted over by about 5ms. Nothing was present in the profiler for that little early bit that was taking 5ms more time. I don't know if the profiler wasn't looking at the right thread or the sample rate wasn't high enough or what.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: