1921426 - Opening tabs with unknown protocols via WebExtension API in Firefox no longer shows "problem loading page" tab, tabs.onUpdated events missing

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[John Bieling]

This is the code which reproduces the reported issue:

let urlSeen = false;
const updateListener = (tabId, changeInfo, tab) => {
    console.log({ tabId, url: changeInfo.url, status: changeInfo.status, urlSeen, changeInfo })
    if (changeInfo.url == "ext+test:1234-1") {
        urlSeen = true;
    }
    if (urlSeen && changeInfo.status == "complete") {
        browser.tabs.onUpdated.removeListener(updateListener);
        console.log("Done");
    }
};
browser.tabs.onUpdated.addListener(updateListener);
browser.tabs.create({ url: "ext+test:1234-1" });

In Firefox ESR 128, this will open a "Problem loading page" tab. The onUpdated listener will report the following:

• { url: undefined, status: "loading" }
• { url: undefined, status: "complete" }
• { url: undefined, status: undefined }
• { url: "ext+test:1234-1", status: "complete" }
• Done

In Firefox Daily 132, this will open a blank page. The onUpdated listener will report the following:

• { url: undefined, status: "loading" }
• { url: undefined, status: "complete" }

Comment 1

[John Bieling]

Attachment: page128.png

The page opened in Firefox ESR 128

Comment 2

[John Bieling]

Attachment: page132.png

The blank page opened in Firefox Daily 132

Comment 3

[John Bieling]

Result from mozregression:

 9:11.49 INFO: Narrowed integration regression window from [fe60d950, 946426bc] (3 builds) to [f4d51319, 946426bc] (2 builds) (~1 steps left)
 9:11.49 INFO: No more integration revisions, bisection finished.
 9:11.49 INFO: Last good revision: f4d513197d9722318981be62da7109e26e04de46
 9:11.49 INFO: First bad revision: 946426bcca27ec5780b0e0f65b36a5b6373f6c06
 9:11.49 INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f4d513197d9722318981be62da7109e26e04de46&tochange=946426bcca27ec5780b0e0f65b36a5b6373f6c06

Base on the pushlog, it seems to be Bug 1881037.

The regression is that a WebExtension can no longer identify if a page load has been finished using the tabs.onUpdated event.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1881037

:nika, since you are the author of the regressor, bug 1881037, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

The simplest change for this is probably to allow extension-triggered navigations to cause the unknown protocol error page to appear, which would involve changing this line to the following:

https://searchfox.org/mozilla-central/rev/9fa446ad77af13847a7da250135fc58b1a1bd5b9/docshell/base/nsDocShell.cpp#6138

    if (!info->TriggeringPrincipal()->IsSystemPrincipal() &&
        !BasePrincipal::Cast(info->TriggeringPrincipal())->AddonPolicy()) {

This basically allows extension principals to cause the error page to load. There's a chance this could lead to some weird breakage with an extension loading an unknown protocol document and expecting it to not cause a navigation though, so potentially we'll need to re-add some of the old hacks around replacing the initial document just for the extension principal case.

Comment 7

[Rob Wu [:robwu]]

Nika's suggestion looks good to us. John, would you be interested in putting up a patch and test?

The original patch's commit message states that it was intended to be limited to web-triggered loads. In this case, the extension-triggered loads are mimicking the user attempting to navigate somewhere from the location bar. If we show an error page in that case, we should continue to do so when tabs.create is called.

John, does webNavigation.onErrorOccurred report the expected URL?

Comment 8

[John Bieling]

(In reply to Rob Wu [:robwu] from comment #7)

Nika's suggestion looks good to us. John, would you be interested in putting up a patch and test?

Yes. I am already building :-)

John, does webNavigation.onErrorOccurred report the expected URL?

Will check and get back to you.

Comment 9

[John Bieling]

Attachment: Bug 1921426 - Restore tabs.onUpdated event for missing protocol handler pages. r=robwu

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1881037

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Hi John, we have 2 betas left this cycle before v132 goes to RC next week. Is this patch still on the radar for landing & uplift this cycle?

Comment 12

[John Bieling]

I have not yet managed to write a test for this and could not complete the patch. I will give it another try on monday.

Comment 13

[Donal Meehan [:dmeehan]]

(In reply to John Bieling (:TbSync) from comment #12)

I have not yet managed to write a test for this and could not complete the patch. I will give it another try on Monday.

:TbSync, Fx133 is now in beta. Any concerns with having something in time for Fx133 release?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4508ca8848bb
Restore tabs.onUpdated event for missing protocol handler pages. r=robwu,smaug

Comment 15

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/4508ca8848bb

Comment 16

[John Bieling]

Comment on attachment 9428265 [details]
Bug 1921426 - Restore tabs.onUpdated event for missing protocol handler pages. r=robwu

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: The regression introduced by Bug 1881037 (which was uplifted to ESR) will impact Beta and ESR users: A missing WebExtension protocol handler will not display the error page (https://bug1921426.bmoattachments.org/attachment.cgi?id=9427716) but a blank page. The WebExtension tabs.onUpdated() event will not report "complete" for the handler page.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: https://bugzilla.mozilla.org/show_bug.cgi?id=1921426#c0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The original regressor added a strict check. This patch relaxes the condition in a very specific case (specific to extensions). The exact behavior is covered by unit tests.
• String changes made/needed:
• Is Android affected?: No

Comment 17

[John Bieling]

Comment on attachment 9428265 [details]
Bug 1921426 - Restore tabs.onUpdated event for missing protocol handler pages. r=robwu

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The regression introduced by Bug 1881037 (which was uplifted to ESR) will impact ESR users.
• User impact if declined: A missing WebExtension protocol handler will not display the error page (https://bug1921426.bmoattachments.org/attachment.cgi?id=9427716) but a blank page. The WebExtension tabs.onUpdated() event will not report "complete" for the handler page.
• Fix Landed on Version: 134
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The original regressor added a strict check. This patch relaxes the condition in a very specific case (specific to extensions). The exact behavior is covered by unit tests.

Comment 18

[Rob Wu [:robwu]]

I'll help with the risk evaluation (you can edit the uplift form from Phabricator):

• Risk to taking this patch: Low
  Why is the change risky/not risky? (and alternatives if risky): The original regressor added a strict check. This patch relaxes the condition in a very specific case (specific to extensions). The exact behavior is covered by unit tests.

Comment 19

[Donal Meehan [:dmeehan]]

Comment on attachment 9428265 [details]
Bug 1921426 - Restore tabs.onUpdated event for missing protocol handler pages. r=robwu

Fx133 is now in RC week.
It's too late for a beta uplift request. Changing the request to release to keep it on the radar for a potential ride along.

Comment 20

[John Bieling]

(In reply to Donal Meehan [:dmeehan] from comment #19)

Comment on attachment 9428265 [details]
Bug 1921426 - Restore tabs.onUpdated event for missing protocol handler pages. r=robwu

Fx133 is now in RC week.
It's too late for a beta uplift request. Changing the request to release to keep it on the radar for a potential ride along.

Is manual uplift to release needed, or will it get merged automatically at some time?

Comment 21

[Donal Meehan [:dmeehan]]

(In reply to John Bieling (:TbSync) from comment #20)

(In reply to Donal Meehan [:dmeehan] from comment #19)

Comment on attachment 9428265 [details]
Bug 1921426 - Restore tabs.onUpdated event for missing protocol handler pages. r=robwu

Fx133 is now in RC week.
It's too late for a beta uplift request. Changing the request to release to keep it on the radar for a potential ride along.

Is manual uplift to release needed, or will it get merged automatically at some time?

This is a manual process. Release uplift timing depends on a few factors.

• If we need an RC respin we review release uplift requests that are suitable ride-along
• If we have an unplanned dot release we review release uplift requests that are suitable ride-along
• We have a scheduled dot release that covers the rest, for Fx133 that is scheduled for 2024-12-10

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:TbSync, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox133 to wontfix.

For more information, please visit BugBot documentation.

Comment 23

[Donal Meehan [:dmeehan]]

:TbSync, I see you set Fx133 to wontfix. There is a release uplift that targets a 133 dot release.
Do you still want to consider this or would you prefer it ride the trains with Fx134?

Comment 24

[John Bieling]

Uhm, I may have not fully understood your release cycle. Uplift to 133 Beta (which would allow us to get it on ESR faster) was not possible, because 133 Beta is a release candidate already, so we will get it into Beta with the 134 merge. Correct?

The bot found the status flag for 133 as optional and pinged me, but because it is too late, I have set it now to wontfix.

In addition to Beta, you also have the Release channel. If the patch is not uplifted to 133 Beta, can we still uplift it to 133 Release? Would that not introduce inconsistency? If you do not have any issues with this thing hitting the release channel before hitting Beta, I am fine with uplifting it to release 133.

Comment 25

[Donal Meehan [:dmeehan]]

Uplift to 133 Beta (which would allow us to get it on ESR faster) was not possible, because 133 Beta is a release candidate already, so we will get it into Beta with the 134 merge. Correct?

Correct, this will ride the train to beta with Fx134 next week.
We will also merge to ESR128 after the end of this week. We already have release candidate for 128.5esr so it was too late for that version but we can pick it up for 128.6esr.

In addition to Beta, you also have the Release channel. If the patch is not uplifted to 133 Beta, can we still uplift it to 133 Release? Would that not introduce inconsistency? If you do not have any issues with this thing hitting the release channel before hitting Beta, I am fine with uplifting it to release 133.

Fx133 has ridden the train from beta to release. However, we still ship dot releases of Fx133 from the release.
The planned dot release for Fx133 is scheduled for 2024-12-10. This patch would have some time in beta with Fx134 before we prepare a Fx133 dot release.

Comment 26

[John Bieling]

Ok, setting fx133 back to fix-optional, as it was before. approval-mozilla-release was already requested.

Comment 27

[Donal Meehan [:dmeehan]]

Comment on attachment 9428265 [details]
Bug 1921426 - Restore tabs.onUpdated event for missing protocol handler pages. r=robwu

Approved for 133.0.3

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/162b76a12aee

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9428265 [details]
Bug 1921426 - Restore tabs.onUpdated event for missing protocol handler pages. r=robwu

Approved for 128.6esr.

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/ce25b84d4d6f