Filed by: mkmelin+mozilla [at] iki.fi
Parsed log: https://treeherder.mozilla.org/logviewer.html#?job_id=299406854&repo=comm-central
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Cub7hhsTQByqYzNG3r9Lww/runs/0/artifacts/public/logs/live_backing.log

Many similar failures. comm/mail/test/browser/message-header/browser_phishingBar.js is another one

These are from bug 1632098 https://hg.mozilla.org/mozilla-central/rev/43b914eee3e3e9fca5986ce6791d35939905f50f

Going to land as bustage fix.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/353338823ec9
set browser.tabs.documentchannel.ppdc false (tests opening new windows broken without it). rs=bustage-fix

This fixes also the Feed message shown as web page. With the pref set to true it shows only garbage.

This is acceptable in the short term, although the breaking changes have been switched off again for now, but we're going to have to fix it properly.

I've managed to fix most of the tests with the pref turned on, through a variety of hacks, but mostly by taking them off our ancient system for waiting for windows to open. See https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=95e20c341344fc3a41239bca7f7e3eefff406835.

Some of the tests are crashing persistently, at mozilla::net::ParentProcessDocumentChannel::DisconnectChildListeners(nsresult, nsresult).

It seems that the event loops of new windows completely stall unless I call setTimeout on them. No matter how long you wait, nothing happens. This could be something to do with the remains of Mozmill hanging around, but I don't think it is. Some of the timeouts I added aren't necessary with the pref set to false.

(In reply to Geoff Lankow (:darktrojan) from comment #7)

I've managed to fix most of the tests with the pref turned on, through a variety of hacks, but mostly by taking them off our ancient system for waiting for windows to open. See https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=95e20c341344fc3a41239bca7f7e3eefff406835.

Some of the tests are crashing persistently, at mozilla::net::ParentProcessDocumentChannel::DisconnectChildListeners(nsresult, nsresult).

A regression was introduced over the weekend that broke the Parent Process DocumentChannel, I pushed the fixes earlier today.

Sorry for that. The pref being disabled by default means that it's not tested.

I pushed the code in order to avoid having to rebase constantly which on this heavily worked on code takes a considerable amount of time.

It seems that the event loops of new windows completely stall unless I call setTimeout on them. No matter how long you wait, nothing happens. This could be something to do with the remains of Mozmill hanging around, but I don't think it is. Some of the timeouts I added aren't necessary with the pref set to false.

Do you have some examples of tests you can link to? Happy to have a look at them.
I've fixed LOTS of old tests so far.

Unfortunately, non-e10s config isn't something we test much these days, the only code we have that runs in non-e10s are the accessibility tests. I certainly paid no time making sure it still worked.
We may have to disable DC if non-e10s. It doesn't bring much value without e10s

I'm hoping PPDC will be enabled by default tomorrow (I've pushed a pref change)

(In reply to Jean-Yves Avenard [:jya] from comment #8)

We may have to disable DC if non-e10s. It doesn't bring much value without e10s

This is probably the easiest thing to do, either by keeping the pref or hard-wiring it.

ISTR you told me the pref would be around until at least the ESR. If that's the case, from our point of view it would be best to call this bug fixed and deal with it again, if we have to, after shipping 78.

(In reply to Geoff Lankow (:darktrojan) from comment #10)

(In reply to Jean-Yves Avenard [:jya] from comment #8)

We may have to disable DC if non-e10s. It doesn't bring much value without e10s

This is probably the easiest thing to do, either by keeping the pref or hard-wiring it.

ISTR you told me the pref would be around until at least the ESR. If that's the case, from our point of view it would be best to call this bug fixed and deal with it again, if we have to, after shipping 78.

I'd like to point out that DC only expose more obviously existing event handling issues.
Intermittent failures that would be made to permanently failing.

These are current, open bugs with a Severity of critical. The Severity of these bugs is being changed to S2 to be consistent with the May 4 2020 Severity definitions.

Please let Release Management know if these bugs are still S2.

This isn't responsible for any of the other failures in the tree, so I'm calling it done.

Okay, it is caused by bug 1654922, even though I did a Try run with that bug backed out and it still failed.

So what do we do? This change here is breaking a number of things in Thunderbird. I suspect that's because we're non-e10s, but TBH I don't know enough about these things to be sure.

I'm going to wait for :jya or :mattwoodrow to pitch in, but a week before I landed my patches for removing DC prefs, I filed bug 1655616 - "Investigate breakage caused by removing browser.tabs.documentchannel related prefs."

What are the remaining issues left broken in Thunderbird?

We're moving towards only supporting DocumentChannel style navigations, and significantly refactoring the code around that, so supporting disabling the pref isn't a workable long term option unfortunately.

(In reply to Anny Gakhokidze [:annyG] from comment #18)

I'm going to wait for :jya or :mattwoodrow to pitch in, but a week before I landed my patches for removing DC prefs, I filed bug 1655616 - "Investigate breakage caused by removing browser.tabs.documentchannel related prefs."

You did, and I apologise for nobody getting to it earlier. The last few weeks have been very busy for all of us.

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

What are the remaining issues left broken in Thunderbird?

We're moving towards only supporting DocumentChannel style navigations, and significantly refactoring the code around that, so supporting disabling the pref isn't a workable long term option unfortunately.

Best I can really do is point you to the tests we have that are now failing and tell you what I know about them. There's a lot of old code here that I don't understand, I'm just a guy trying to keep our tree green.

• test_ext_contentscript_triggeringPrincipal.js has a crash when run with --tag in-process-webextensions that wasn't there before. Oddly, I tried to run this one with Firefox and it stops before even getting to the crash.
• browser_jsContentPolicy.js has a <noscript> block that should be displayed but isn't. We've got a content policy that blocks javascript in this document.
• browser_vcardActions.js has a protocol handler (we define the addbook protocol) and a content handler involved. I'm not sure which is failing to work.

I don't believe I can be of much help here.

When I last look at it; some of the tests was due to the incorrect assumptions that when you open a tab/window you can immediately read its content within the same event loop; when it should be waiting for the "load" event to be fired.

Like this code here looks suspicious to me:
https://hg.mozilla.org/comm-central/file/tip/mail/test/browser/message-header/browser_phishingBar.js#l251

Or in this code, there's a proper wait here https://searchfox.org/comm-central/source/mail/test/browser/content-policy/browser_jsContentPolicy.js#258-266

but not for this one:
https://searchfox.org/comm-central/source/mail/test/browser/content-policy/browser_jsContentPolicy.js#143-167

this looks suspicious. Mind you, this is just glancing at the code and I can't be certain that this is correct.

However, this is where I would start looking.

The push above doesn't solve all of the problems, but it does help.

This is identified as a regression to bug 1657493 - every news article crashes.

Is there a super quick thing we can do to fix beta?

I sent a band-aid fix to bug 1657493.

(In reply to Wayne Mery (:wsmwk) from comment #25)

This is identified as a regression to bug 1657493 - every news article crashes.

Not every news article. Only new ones.
If "Select Newsgroup for offline use" is activated, articles from the local datastore can be shown as normal.

It it important for this bug (or some other) to be resolved because of the news breakage.

And I miscalculated in not taking the band-aid patch from bug 1657493 for 81.0b2 because I thought every article crashed. Will take it in the next beta - but I hope the larger issue will already be fixed by then. Because most users are not going to realize that they can do "Select Newsgroup for offline use". Further, most users likely won't care because they are going to news to get new messages, not the old ones.

I think we did all we needed here. Certainly there's always room for test improvements, but nothing really actionable in this bug.