1494436 - [Linux][Nightly] I can no longer open external links in Nightly after the 2nd selection of the "Use Nightly as my default browser"

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect, P3)

Description

[Paul_B]

Attachment: Short video of the issue

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:64) Gecko/20100101 Firefox/64
Build ID: 20180926132058 / Version: 64.0a1

Steps to reproduce:

1. Open Nightly and select the "Use Nightly as my default browser" when prompted.
2. Close the browser.
3. Navigate to Thunderbird and select an external link.
4. After the selected link is opened in Nightly without any issues, select the same option as in step 1 when prompted.
5. Navigate back to Thunderbird and select a link again.
6. Close the browser and repeat step 5. 

Actual result: 

The links are no longer opened starting with the 2nd selection of the "Use Nightly as my default browser". Once the browser is closed, it can no longer be opened by selecting a link. It can be opened only from the .exe file. I have attached a file with a short video for better understanding.

Expected result: 

The links are opened without any issues in Nightly as long as this is the default selected browser.

Comment 1

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

I can reproduce this with Ubuntu's Thunderbird 68.2.2 and Nightly 75. If we are launched by clicking a link in Thunderbird, clicking "Use as my default browser" actually sets the xdg default-web-browser to thunderbird.desktop, and then Thunderbird refuses to open those links (I see a "https://www.mozilla.org" is not a valid feed." message in Thunderbird's status bar).

This is because when Nightly is launched from Thunderbird, the MOZ_APP_LAUNCHER environment variable is still pointing at Thunderbird. The shell service uses that to decide mAppPath, so any further work we do with checking or setting ourselves as a handler will use Thunderbird's path and app info.

There's a few other places that use this as well, and I think it's going to mess up at least the crash reporter.

Debian's Thunderbird sets this in the wrapper.sh, I think in order to make sure that the wrapper is used to launch instead of the binary.

Probably the best thing to do would be for Thunderbird to clear this var when it shells out. I think that this could be done by passing a context to g_app_info_launch_uris(), and it's possible to use g_app_launch_context_unsetenv(), but I'm not sure where to get a context from. I don't know enough gnome to follow this any further at the moment.

Comment 4

[bitter.taste]

but I'm not sure where to get a context from

Using g_app_launch_context_new:

GAppLaunchContext *ctx = g_app_launch_context_new();
if (!ctx) return NS_ERROR_FAILURE;
g_app_launch_context_unsetenv(ctx, "MOZ_APP_NAME");
g_app_launch_context_unsetenv(ctx, "MOZ_APP_LAUNCHER");
g_app_launch_context_unsetenv(ctx, "MOZ_LIBDIR"); // This too?

This context should be passed to the g_app_info_launch_default_for_uri and g_app_info_launch_uris calls.

Comment 5

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to bitter.taste from comment #4)

Using g_app_launch_context_new:

This context should be passed to the g_app_info_launch_default_for_uri and g_app_info_launch_uris calls.

Thanks for the pointers! I'm setting the priority to indicate it's fallen into the backlog, but it would be good to fix here (with tests) so Thunderbird can pick it up from the common Toolkit.

Comment 6

[Simon McVittie]

Here is a tested patch based on the comments above: https://people.debian.org/~smcv/Bug-1494436-Unset-MOZ_APP_LAUNCHER-for-external-MIME-hand.patch

Sorry, I am not in a position to write automated tests for this (or even to know whether it's feasible to do so), but it has the desired result when applied as a patch to Debian's thunderbird package, version 1:91.6.2-1. I tested with Debian's firefox-esr package as the web browser, but newer Firefox branches probably have similar behaviour.

Steps to reproduce the issue in a test account:

• use an expendable user ID or a VM, because these test steps are destructive
• Debian testing/unstable system (I used a virtual machine with GNOME)
• firefox-esr as default web browser (this is the default in Debian GNOME)
• rm -f ~/.config/mimeapps.list
• Completely exit from both Firefox and Thunderbird
• strace -eexecve -ff thunderbird
• no need to set up an actual email account
• press Alt, Help -> About Thunderbird, click on Privacy Policy
  • it launches firefox as a subprocess (you'll see this in the strace)
• If Firefox asks to be made the default browser:
  • click "Make Firefox my default browser"
• Completely exit from both Firefox and Thunderbird again
• cat ~/.config/mimeapps.list
• xdg-open http://example.com

Expected result: mimeapps.list doesn't exist, is empty, or lists firefox-esr as default web browser. example.com opens in Firefox.

Actual result: mimeapps.list has thunderbird.desktop as default for HTML, HTTP etc., and Thunderbird opens instead of Firefox.

Mitigation: If firefox-esr is already running before you click the link in Thunderbird, then the firefox-esr subprocess of Thunderbird just sends an IPC request to the existing firefox-esr instance and then exits, so people who habitually have a web browser open at all times will often not experience this bug (which is presumably why Debian's Thunderbird maintainers didn't experience this).

Comment 7

[Simon McVittie]

Attachment: Unset MOZ_APP_LAUNCHER for external MIME handlers

Comment 8

[Simon McVittie]

Attachment: Bug-1494436-Unset-MOZ_APP_LAUNCHER-for-external-MIME-hand.patch

Sorry, the previously-attached patch was an outdated version. Try this one instead.

Comment 9

[Kirk Steuber (he/him) [:bytesized]]

I don't really know enough about what's going on here to say much about this patch with any confidence.

@jhorak - It looks like you wrote most of this file, would you be available to look at this patch?

Comment 10

[[e-mail] (:bhearsum)]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #9)

I don't really know enough about what's going on here to say much about this patch with any confidence.

@jhorak - It looks like you wrote most of this file, would you be available to look at this patch?

Emilio, I see you've been in this code fairly recently as well -- any chance you're comfortable reviewing this?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Yes, the patch looks reasonable to me, it is harmless at best. I have some nits that I have addressed locally, but I can't put myself as the reviewer of the revision because Phabricator thinks I authored it. So I think I'll do the manual push dance for this one.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58d678ccbab1
Unset MOZ_APP_LAUNCHER for external MIME handlers. r=emilio

Comment 13

[[e-mail] (:bhearsum)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

Yes, the patch looks reasonable to me, it is harmless at best. I have some nits that I have addressed locally, but I can't put myself as the reviewer of the revision because Phabricator thinks I authored it. So I think I'll do the manual push dance for this one.

Thanks!

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/58d678ccbab1

Comment 15

[Simon McVittie]

Thanks for adjusting and applying this.

• gboolean result = g_app_info_launch_uris(

•  mApp, &uris, GetLaunchContext().get(), getter_Transfers(error));
  

Just to be sure: is RefPtr "smart" enough that it will ensure the GObject reference to the GAppLaunchContext is not released until after the call to g_app_info_launch_uris(), even though there is no variable in-scope to own the reference?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Yes, C++ lifetime extension rules guarantee that, I'm pretty sure. I'm on my phone now but I can try to give a proper standard reference tomorrow.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

I think the "Temporary object lifetime" section in https://en.cppreference.com/w/cpp/language/lifetime is probably a decent enough reference (I'd look the specific spec text but... :))