[Linux][Nightly] I can no longer open external links in Nightly after the 2nd selection of the "Use Nightly as my default browser"
Categories
(Firefox :: Shell Integration, defect, P3)
Tracking
()
People
(Reporter: paul.boiciuc, Assigned: smcv)
References
Details
Attachments
(2 files, 1 obsolete file)
101 bytes,
text/plain
|
Details | |
3.35 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•2 years ago
|
Comment 1•2 years ago
|
||
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•1 year ago
|
||
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•1 year ago
|
||
(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
andg_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.
Updated•1 year ago
|
Assignee | ||
Comment 6•3 months ago
|
||
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).
Assignee | ||
Comment 7•3 months ago
|
||
Assignee | ||
Comment 8•2 months ago
|
||
Sorry, the previously-attached patch was an outdated version. Try this one instead.
Comment 9•2 months ago
|
||
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•1 month ago
|
||
(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•1 month ago
•
|
||
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•1 month ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/58d678ccbab1 Unset MOZ_APP_LAUNCHER for external MIME handlers. r=emilio
Comment 13•1 month ago
|
||
(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•1 month ago
|
||
bugherder |
Updated•1 month ago
|
Assignee | ||
Comment 15•1 month ago
|
||
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•1 month ago
|
||
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•1 month ago
|
||
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... :))
Updated•1 month ago
|
Description
•