1667774 - HTTP Refresh no longer work in content tabs

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, defect)

Description

[[e-mail]]

Bug 511240 added a progress listener to Thunderbird's content tabs. Unfortunately, it doesn't quite honour the API correctly, which means that it ends up blocking all HTTP Refresh redirects.
Bug 1455471 then duplicated the error with its own progress listener. (This error could possibly have been avoided by comparing with Firefox's tabs progress listener.)

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Geoff Lankow (:darktrojan)]

Comment on attachment 9178207 [details] [diff] [review]
Proposed patch

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

Ah, the old "all of these methods are void, except one" trick! Thanks for finding and fixing this. r+ with a nit fixed.

::: mail/base/content/tabmail.js
@@ +1947,4 @@
>        for (let listener of this.mTabsProgressListeners.values()) {
>          if (aMethod in listener) {
>            try {
> +            if (!listener[aMethod](...aArgs))

You'll need these here: {}

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9178207 [details] [diff] [review]
Proposed patch

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

::: mail/base/content/tabmail.js
@@ +1947,4 @@
>        for (let listener of this.mTabsProgressListeners.values()) {
>          if (aMethod in listener) {
>            try {
> +            if (!listener[aMethod](...aArgs))

I'd also just return false here and not bother having an rv set. (Then return true down there also of course)

Comment 4

[[e-mail]]

Attachment: Addressed review comment

(In reply to Geoff Lankow from comment #2)

You'll need these here: {}

Sorry, my mach lint complained about protovis and I hadn't realised that meant it had actually skipped tabmail.js.

(In reply to Magnus Melin from comment #3)

I'd also just return false here and not bother having an rv set. (Then return true down there also of course)

I was copying Firefox's code here; I think they want to call all of the listeners, and then return false if any of them did.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d47938982f6b
Fix HTTP refresh for content tabs r=darktrojan

Comment 6

[[e-mail]]

Thanks for checking this in. Is this change eligible for Thunderbird 78 uplift via comm-beta?

Comment 7

[Magnus Melin [:mkmelin]]

I don't see why not. Do set the approval request flags.

Comment 8

[Geoff Lankow (:darktrojan)]

It will need to be reworked for 78.

Comment 9

[[e-mail]]

Comment on attachment 9178999 [details] [diff] [review]
Addressed review comment

(In reply to Geoff Lankow from comment #8)

It will need to be reworked for 78.

I take it that means that this version should already apply to c-b? I can easily write a backport for 78.

[Approval Request Comment]
Regression caused by (bug #): bug 511240, bug 1455471
User impact if declined: Some sites don't refresh correctly when opened in content tabs
Risk to taking this patch (and alternatives if risky): Low, only affects content tabs

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9178999 [details] [diff] [review]
Addressed review comment

[Triage Comment]
Approved for beta

Comment 11

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/0a0f38a136e8

Comment 12

[Magnus Melin [:mkmelin]]

Neil, can you attach a backport and request 78 approval?

Comment 13

[[e-mail]]

Attachment: ESR 78 backport

[Approval Request Comment]
Regression caused by (bug #): bug 511240, bug 1455471
User impact if declined: Some sites don't refresh correctly when opened in content tabs
Risk to taking this patch (and alternatives if risky): Low, only affects content tabs

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9190314 [details] [diff] [review]
ESR 78 backport

[Triage Comment]
Approved for esr78

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 78.6.0:
https://hg.mozilla.org/releases/comm-esr78/rev/1def987eea61