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

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Location Bar
P3
normal
RESOLVED FIXED
3 years ago
7 months ago

People

(Reporter: dbaron, Assigned: Gijs)

Tracking

({regression})

Trunk
Firefox 52
x86_64
Linux
regression
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Created attachment 8411274 [details]
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.
(Reporter)

Updated

3 years ago
Keywords: regression, regressionwindow-wanted
(Reporter)

Comment 1

3 years ago
One day regression range from 64-bit Linux mozilla-central nightlies:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4ffb23062b3b&tochange=b48e06621dc9
Keywords: regressionwindow-wanted
(Reporter)

Comment 2

3 years ago
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?
Blocks: 846635
Flags: needinfo?(raymond)
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.
Flags: needinfo?(raymond)
Flags: needinfo?(mano)
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
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?
yes, comment 4 sounds like a good suggestion to try.
Flags: needinfo?(mano)
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Comment 7

8 months ago
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 8

8 months ago
(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

8 months ago
mozreview-review
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?
Attachment #8796143 - Flags: review?(mak77)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

8 months ago
mozreview-review-reply
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.)
(Assignee)

Comment 12

8 months ago
mozreview-review-reply
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

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

https://reviewboard.mozilla.org/r/82100/#review81418
Attachment #8796143 - Flags: review?(mak77) → review+

Comment 14

8 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/adb484f84dec
stop races in location bar <return> handling code, r=mak

Comment 15

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/adb484f84dec
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Updated

8 months ago
Duplicate of this bug: 1279489

Updated

7 months ago
Depends on: 1313403
(Assignee)

Updated

7 months ago
Depends on: 1315944
(Assignee)

Updated

7 months ago
Depends on: 1315948
You need to log in before you can comment on or make changes to this bug.