1646483 - Port Bug 1644943 to Thunderbird: Create single webprogress for CanonicalBrowsingContext, regardless of process the browser element contents are in

Status: RESOLVED FIXED

(Thunderbird :: General, task, P1)

Description

We need to port bug 1644943 - looks like it's blowing up a whole bunch of our tests.

https://hg.mozilla.org/mozilla-central/rev/ffe294ce39df

Comment 1

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9bce1490f4dd
Port Bug 1644943 to Thunderbird: Create single webprogress for CanonicalBrowsingContext. rs=bustage-fix

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/20749cb17aec
Fix WindowHelpers._wait_for_generic_load. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/8bae3d95175d
Fix some problems, but not others, in chat. rs=bustage-fix

Comment 3

[Geoff Lankow (:darktrojan)]

I think with these three patches landed all of the tests will pass again. Chat is currently broken (for possibly unrelated reasons, see bug 1646611) and the chat preview in the preferences window is disabled (for related reasons, I'm not sure what to do with it).

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1646483-webprogress-all.diff

This should be reviewed, even though it's already landed. There's follow-up bugs for the broken chat things.

Comment 5

[alta88]

IMHO, rather than working around the chat tab not conforming in core tab code, you should make chat tab conform. Meaning it should always have a browser member on tabInfo and always return one from getBrowserForTab(), like contentTab type, and when chat url is loaded it must have a <browser> up front. That's why I did this, to at least get the <browser> after the <chat-conversation> is created:
https://hg.mozilla.org/releases/comm-beta/rev/c84786d3659a#l1.132

The glodaFacet has a similar issue, but it's due to timing rather than no <browser> ever. And calendar and task tabs, being xul, never have a browser. Anyway, you're just going to keep chasing your tail unless all tabs in Tb conform to what a tab is expected to contain for WE in Fx.

Comment 6

[Intermittent Failures Robot]

7 failures in 4035 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 7

Platform breakdown:

• linux64: 7

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1646483&startday=2020-06-15&endday=2020-06-21&tree=all

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9157806 [details] [diff] [review]
1646483-webprogress-all.diff

Review of attachment 9157806 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe you can take a stab at fixing it as per alta88's comment.

::: mail/components/preferences/messagestyle.js
@@ +222,5 @@
>      document.getElementById("previewDeck").selectedIndex = 1;
>    },
>  
>    reloadPreview() {
> +    // this.browser.init(this.conv);

So this is bug 1646874?

Comment 8

[Geoff Lankow (:darktrojan)]

To be honest, I don't know how alta88's comment relates to what was needed here. The progress listener in the chat code has nothing to do with the other progress listeners that I've fixed here, or with WebExtensions.

Comment 9

[alta88]

Yes, and that's the problem. A WE cannot expect all loads, in tabInfo.browser, in a Tb tab to emit progress events in the same way. It also cannot even expect a |browser|. A chat tab that does not autoconnect an account, for example, has no |browser| and only adds one to a <deck> later; completely unnecessary and hard to set up for, and why you have to comment out tests. And why shouldn't a chat and every other tab emit WE api progress events. The glodaFacet tab is similar; the <browser> is not there on tab open and is additionally wrapped in some <iframe> for zoom purposes, again unnecessarily.

You do realize, I hope, that there is the tabmail.progresslistener (which wasn't even being added to firstTab before) which you've now made TabProgressListener and the |tabProgressListener| that applies only to contentTab types in specialTabs.js. So what I'm saying, clearly enough I believe, is that you should step back and audit all tab types for complete compatibility with WE tab apis, consistent non duplicated progresslisteners, and the expectation that a Firefox extension operating on tab <browser> can run cleanly in Tb. This includes #messagepane and #multimessage <browser>s, and chat and glodaFacet and even chromeTab. Because what I'm seeing is a lot of bandaid layering going on.

Note that web progress was recently broken for WE in Tb until Bug 1633011, in case you're not aware.

Anyway. What I've needed to do I've done in BrowseInTab, using custom progress listeners, and what made sense only to do internally I did in Bug 1641345.

Comment 10

[Geoff Lankow (:darktrojan)]

Comment on attachment 9157806 [details] [diff] [review]
1646483-webprogress-all.diff

I hear what you're saying, alta88, but *this* bug is only about making sure that everything works again, like it did before bug 1644943 happened. I'll copy your comments into a new bug.