1000458 - enter in location bar (URL bar) for bookmark keyword immediately followed by opening new tab sometimes loads url in new tab instead of old, particularly under high load

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[David Baron :dbaron:]

Attachment: load simulator

I've been seeing this bug for about six months, I think, but I realized I never actually filed it.  (I think I kept thinking I'd find better steps to reproduce or a reliable regression range.  I just came up with better steps to reproduce, but I haven't yet bothered with the regression range.)  But I see it in my real browsing at least a few times a week, and it's mildly annoying.

Steps to reproduce (real world):
 1. have a Firefox session with a lot of tabs open, some of which are running a bit of javascript and which thus sometimes pauses
 2. open a new tab
 3. type something in the URL bar that involves a bookmark keyword, hit enter, and *immediately* press Ctrl+T to open a new tab

Actual results: the thing typed in the URL bar frequently opens in the new tab instead of the old one

Expected results: it should open in the tab whose URL bar it was typed in

Steps to reproduce (simulated, starting from a clean firefox profile):
 1. bookmark this page
 2. edit the bookmark to replace the name with "bug", replace the bug number in the URL with %s, and add the keyword "bug"
 3. load the attached load simulator in a tab
 4. open a new tab with Ctrl+T
 5. wait for this new tab to fully load an the caret to appear in the location bar [if you don't do this, the symptoms are different but also buggy!]
 6. type "bug 4000" and quickly press Enter and then Ctrl+T (the Enter and Ctrl+T have to be within the same tick of the 2s timer in the load simulator)

Actual results: bug 4000 is loaded in the new (current) tab

Expected results: bug 4000 is loaded in the previously-current tab, and the current tab is blank

I'm seeing this in a 64-bit Linux debug build, and have been for (very) approximately six months.

Comment 1

[David Baron :dbaron:]

One day regression range from 64-bit Linux mozilla-central nightlies:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4ffb23062b3b&tochange=b48e06621dc9

Comment 2

[David Baron :dbaron:]

https://hg.mozilla.org/mozilla-central/rev/19845a51b863 seems like by far the most likely candidate, although I haven't verified by local backout.

Does that make sense?

Comment 3

[Marco Bonardo [:mak]]

I think makes sense. I don't know if raymond is still following development, mano reviewed the patch though so flipping the needinfo.

I think this case is very likely, we are asynchronously getting the charset, and then proceeding with the requested operation. I see some check was added to avoid overwriting a location change, but sounds like it's not precise enough.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Drive-by analysis:

If the async code is resolving and just operating on the gBrowser.selectedBrowser, could we not instead have the async code remember which browser was selected at the time of the start of the query, and just target it (if it exists), once it resolves?

Comment 5

[Marco Bonardo [:mak]]

yes, comment 4 sounds like a good suggestion to try.

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1000458 - stop races in location bar <return> handling code,

Review commit: https://reviewboard.mozilla.org/r/82100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82100/

Comment 7

[:Gijs (he/him)]

There's quite a few info() statements in the test I included, mostly because I ran into a test timeout (fixed now) and what with the yields and the relatively few assertions in the test itself, mostly you would want to know where the test "got stuck", and so I disambiguated that by inserting info() statements between the yields. While I could get rid of them again, if the test does end up being intermittent I expect they'll be useful in diagnosing why that is the case, and otherwise they don't hurt (automation doesn't include test assertions or info() in its output unless the test fails) so I just left them.

Comment 8

[:Gijs (he/him)]

(I picked this up because I ran into it when looking for "good first" location bar bugs, and I realized that I'd seen bugs about this behaviour before, but never with a clear idea of why that would be happening or how to fix - but the testcase here was much more straightforward and there was even a suggestion on how to address the problem, so I took the opportunity to do so...)

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8796143 [details]
Bug 1000458 - stop races in location bar <return> handling code,

https://reviewboard.mozilla.org/r/82100/#review80942

I have a doubt regarding the utilityOverlay change, could you please check that?

::: browser/base/content/test/urlbar/browser_urlbarRaceWithTabs.js:18
(Diff revision 1)
> +    url: bookmark.url,
> +    title: bookmark.title,
> +  })).guid;
> +
> +  registerCleanupFunction(function* () {
> +    yield PlacesUtils.bookmarks.remove(bookmarkId);

nit: fwiw you could just pass the bookmark object instead of the guid, so:
let bm = yield ...insert();
yield ...remove(bm);

::: browser/base/content/test/urlbar/browser_urlbarRaceWithTabs.js:38
(Diff revision 1)
> +  let oldTabLoadedPromise = BrowserTestUtils.browserLoaded(oldTab.linkedBrowser, false, kURL);
> +  oldTabLoadedPromise.then(() => info("Old tab loaded"));
> +  let newTabCreatedPromise = BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabOpen");
> +
> +  info("Creating bookmark and keyword");
> +  yield* addBookmark({title: "Test for keyword bookmark and URL", url: kURL, keyword: "urlbarkeyword"});

nit: while it won't hurt, I don't think yield* is needed here, we only care about the generator to be done.

::: browser/base/content/utilityOverlay.js:249
(Diff revision 1)
>    }
>  
> -  var w = getTopWin();
>    if ((where == "tab" || where == "tabshifted") &&
>        w && !w.toolbar.visible) {
>      w = getTopWin(true);

I have some doubts related to this.
For popups we override w, and what was used before your patch was w.gBrowser
now you have w.gBrowser that may refer to a gBrowser and aCurrentBrowser that is a browser that may be in another gBrowser.
And later you do:
w.gBrowser.getTabForBrowser(aCurrentBrowser);
that could arguably fail.

Could we define aCurrentBrowser after w has a stable value, to avoid possibly regressing this code?

Comment 10

[:Gijs (he/him)]

Comment on attachment 8796143 [details]
Bug 1000458 - stop races in location bar <return> handling code,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82100/diff/1-2/

Comment 11

[:Gijs (he/him)]

Comment on attachment 8796143 [details]
Bug 1000458 - stop races in location bar <return> handling code,

https://reviewboard.mozilla.org/r/82100/#review80942

> I have some doubts related to this.
> For popups we override w, and what was used before your patch was w.gBrowser
> now you have w.gBrowser that may refer to a gBrowser and aCurrentBrowser that is a browser that may be in another gBrowser.
> And later you do:
> w.gBrowser.getTabForBrowser(aCurrentBrowser);
> that could arguably fail.
> 
> Could we define aCurrentBrowser after w has a stable value, to avoid possibly regressing this code?

I have done this, and updated some comments to reflect how this works. Though I don't think it's likely to happen that we end up with a different `w` from `aCurrentBrowser.ownerGlobal` the code will now take care of this. (I think it's unlikely because for this to happen we'd have to pass a `currentBrowser` in the params, which we only do from the location bar, which is disabled in the popup window case. Of course, that'd still be a latent bug which would be exposed if other code that *can* be used from the popup window case starts passing this parameter.)

Comment 12

[:Gijs (he/him)]

Comment on attachment 8796143 [details]
Bug 1000458 - stop races in location bar <return> handling code,

https://reviewboard.mozilla.org/r/82100/#review80942

> I have done this, and updated some comments to reflect how this works. Though I don't think it's likely to happen that we end up with a different `w` from `aCurrentBrowser.ownerGlobal` the code will now take care of this. (I think it's unlikely because for this to happen we'd have to pass a `currentBrowser` in the params, which we only do from the location bar, which is disabled in the popup window case. Of course, that'd still be a latent bug which would be exposed if other code that *can* be used from the popup window case starts passing this parameter.)

Eh, to be very precise, that *would have been* a latent bug - it isn't with the adapted code, I believe it should work correctly in this case. Thanks for spotting it!

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8796143 [details]
Bug 1000458 - stop races in location bar <return> handling code,

https://reviewboard.mozilla.org/r/82100/#review81418

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adb484f84dec
stop races in location bar <return> handling code, r=mak

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/adb484f84dec