1777297 - Address bar URLs get incorrectly copypasted as 'about:blank', from broken tabs after a Nightly update is installed in the background

Status: NEW

(Firefox :: Address Bar, defect, P3)

Description

[Daniel Holbert [:dholbert]]

I frequently get Restart to Keep Using Nightly pages in the morning / when an update is available (in my main browsing profile, usually right after I start another parallel Nightly session with a fresh profile, which causes an update to be installed).

When this happens, if I go to any of my broken tabs and try to copy the URL to clipboard, I actually end up with about:blank in my clipboard.

I haven't verified exact STR, but I think it's something like this...

STR:

1. Trigger a situation where you have some broken tabs that have URLs in the address bar that we attempted and failed to load. (I think usually/often I get this for session-restored tabs, which we lazily restore.)
2. Look at one such broken tab.
3. Ctrl+L to focus+select the URL (or click the URL bar and do Ctrl+A)
4. Ctrl+C to copy
5. Go to some other application (e.g. gedit) and do Ctrl+V to paste

ACTUAL RESULTS:
The string about:blank is pasted

EXPECTED RESULTS:
The text that I selected & copied (the contents of my URL bar) should have been pasted.

(Sorry for not having more exact detail; I just hit this again today and figured we should have a bug on file for this, if we don't already.)

Comment 1

[Daniel Holbert [:dholbert]]

Two interesting things I noticed today while poking at this:
(1) Unsurprisingly, this affects the "select / middle-click" copypaste method on Linux as well (which sometimes uses a different clipboard). It's not specific to Ctrl+C/Ctrl+V copypaste flow.

(2) If you select not quite all of the URL (e.g. if you leave off the leading h), then the copypaste operation works correctly. E.g. if the location bar of your broken tab is showing https://www.example.org/ and you select ttps://www.example.org/ and Ctrl+C and then Ctrl+V into gedit, you end up faithfully pasting ttps://www.example.org/. But if you then expand your selection to include the leading h and Ctrl+C to copy, and then Ctrl+V into gedit, you end up pasting about:blank (as described in comment 0).

So it seems like there's some special case code that does something along the lines of "did the user attempt to copy the whole URL? if so, then query the tab for what it's visiting instead of actually copying the text that the user was highlighting". And this special-case codepath falls over and ends up getting about:blank in this special scenario where you have broken tabs due to a background update having installed.

(I realize this is kind of a niche use-case, but IMO it's one that we should address... I usually end up doing some version of the STR if I've got a tab that I'm wanting to interact with & that I'm wary of losing through the restart-browser-after-background-update-was-installed roundtrip, and I copypaste the URL just in case it gets improperly restored. And then I'm stymied by the fact that it gets copypasted as about:blank.)

Comment 2

[Marco Bonardo [:mak]]

yes, if the whole url is selected we copy the currentURI
https://searchfox.org/mozilla-central/rev/4654ce1e24a6af17bc96ab60f1f70c218755142f/browser/components/urlbar/UrlbarInput.jsm#2241-2242
Since we manipulate the url a bit, we want to be sure we get the exact open url.

It sounds like we should detect these cases where we show a special page instead of the actual one.

Comment 3

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

The severity field is not set for this bug.
:adw, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Mark Banner (:standard8)]

(In reply to Marco Bonardo [:mak] from comment #2)

yes, if the whole url is selected we copy the currentURI
https://searchfox.org/mozilla-central/rev/4654ce1e24a6af17bc96ab60f1f70c218755142f/browser/components/urlbar/UrlbarInput.jsm#2241-2242
Since we manipulate the url a bit, we want to be sure we get the exact open url.

To expand on this: It is not enough to only check the pageproxystate status, we should check for other statuses as well.

Comment 5

[:Gijs (away/busy until Mar 10)]

(In reply to Daniel Holbert [:dholbert] from comment #0)

1. Trigger a situation where you have some broken tabs that have URLs in the address bar that we attempted and failed to load. (I think usually/often I get this for session-restored tabs, which we lazily restore.)

This shouldn't really happen in the first place, and we recently tried to make this better (cf. bug 610357, bug 1824040). I know it's been a while, but are you still seeing this? Can you offer STR in a separate bug?

Comment 6

[Daniel Holbert [:dholbert]]

I happen to be currently in approximately the situation here (I have an update that installed in the background, such that Firefox gives me a "Restart Required" error page for https://phabricator.services.mozilla.com/ right now).

Replying to comment 5: the behavior has changed slightly for me, such that the URLbar is now blank for the affected lazily-restored tabs. In particular: I have some lazily-restored background tabs at https://phabricator.services.mozilla.com (that I can find via switch-to-tab), and when I foreground these tabs, the URL bar is blank and the content area says "restart required".

(The URL bar being blank is the new/different thing vs. the description that was quoted in comment 5.)

Is that the current expected outcome?

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)

when I foreground these tabs, the URL bar is blank and the content area says "restart required".

Side note, the blank URL bar for these lazily-restored tabs is a bit disconcerting; it's not obvious to me as a user whether they'll be restored properly when I do accept the update and restart Firefox. (It seems superficially like, in the process of foregrounding these tabs and getting them to render an error page with a blank URL bar, I might be prompting Firefox to forget the lazily-restored URL. I'm not sure if that's actually what's happening or not since I haven't yet restarted, but it feels like that might be what'll happen.)

Also: circling back to the copypaste use-case from comment 0 here: there's still a bit of weirdness there, and I spun off bug 1833686 for the new symptom that I'm seeing.

Comment 8

[:Gijs (away/busy until Mar 10)]

If this is specific to the restart required / IPC version mismatch stuff, I don't know what the current expected outcome is - sorry, but that wasn't obvious to me because I got here from a slack thread that wasn't immediately related to about:restartrequired.

I'm going to forward the needinfo to :mccr8 / :nika who I think know more about this. I think generally the restart required bits are unfortunate but very hard to fix in terms of occurring at all, and I honestly don't know what better behaviour would look like: it would also be misleading if we indicated to the user that the about:restartrequired page was at example.com, because it's not... Hopefully there is code to avoid storing that page in session restore at all, and then I'd expect us to go back to the correct URL after the restart. But I don't know if that's the case today.

Comment 9

[Andrew McCreight [:mccr8]]

I don't know anything about about:restartrequired, sorry, only the low-level issue of needing it.

I see what you mean about not wanting to show the URL if we haven't loaded the page. Maybe the URL could be something like about:restartrequired?page=example.com and then it still restores correctly to example.com, so the user is less alarmed about the state (assuming we're doing it correctly). Or maybe it could pop up a new tab with about:restartrequired? Though that's kind of annoying, too.

Comment 10

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

I think ideally the about:restartrequired would be treated like an error page load, so that the original URL is shown in the URL bar like we do when you load e.g. a certificate error. In fact it looks like that's what the code is trying to do, though it isn't working.

The actual rendering of about:restartrequired is entirely handled by the frontend - we don't actually start rendering it in any of the DOM/IPC code. When the browser reports a crash due to restartrequired, it will be handled in the frontend by sendToRestartRequiredPage (https://searchfox.org/mozilla-central/rev/2ca95198a2a0806de358a0484d96d4354e3cbaab/browser/modules/ContentCrashHandlers.jsm#225), which forces the browser non-remote, and loads a NS_ERROR_BUILDID_MISMATCH into it (https://searchfox.org/mozilla-central/rev/2ca95198a2a0806de358a0484d96d4354e3cbaab/browser/modules/ContentCrashHandlers.jsm#530). (unless we hit bug 1830516 due to it crashing in the background - but I don't think that's the issue here)

Given that the URI is provided there to be rendered, it makes me think that perhaps the crash is happening early enough that the currentURI of the browser has not been updated to the URI which we were trying to load into it. This makes sense, as an about:restartrequired crash will happen when the content process is just launching, so it definitely hasn't had a chance to navigate anywhere yet. Perhaps frontend could keep track of the URL which it was starting the process to try to load for these cases, so we can preserve it as the URL for the error page?

Given that either way we load the page as an error page through the docshell, I think it should avoid being stored in sessionrestore so long as we get the URL which we're pasisng in correct.

Comment 11

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

FYI if someone wants to reproduce with a local build, the strategy I describe in bug 1830516 comment 4 should probably work here as well.