1598212 - Website loads in incorrect tab instead of the previously active tab after creating new tab

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Alice0775 White]

When I test bug Bug 1597076 (but str is not same)

I can reproduce the issue on Linux Mint19 Cinnamon.

Reproducible: 1 of 5

Steps to reproduce:

1. Make sure newtab page is shown in tab
2. Left click item on Top Sites (e.g Wikipedia) in the tab, So that the page will load in current tab.
3. Immediately after the clicking, Create new tab (hit Ctrl+t). So that a newtab page will open in new foreground.
   --- Observe which tab a page is loaded in.
4. If the page loads correctly, repeat step 2 and 3

Actual Results:
By a probability of approximately 1/5, the page loads in the created new tab at step 3.

Expected Results:
The page should load in the tab at step 2.

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=63a0e2f626febb98d87d2543955ab99a653654ff&tochange=d2d518b1f8730eb61554df7179ef9a2aeed4d843

Comment 1

[Alice0775 White]

Suspect:
2d34c4f04bc7e18e0e1d2c77581fcca07fd1e865 Boris Zbarsky — Bug 1457155. Rename various focus manager variables to make it clearer that they're Elements. r=mccr8
eea837f5ac160dcec9ec03cc598acd57ae42a487 Boris Zbarsky — Bug 1457156. Rename nsPIDOMWindow::mFocusedNode to reflect that it's an Element now. r=mccr8

Comment 2

[Boris Zbarsky [:bzbarsky]]

That is ... very odd. Does this step:

Left click item on Top Sites (e.g Wikipedia) in the tab, So that the page will load in current tab.

do something with the selectedBrowser or focused browser or something instead of just navigating in the existing browsing context? Gijs, do you know who would know that?

Comment 3

[:Gijs (he/him)]

The only thing I can reproduce is it loading in a third tab. I end up with [original new tab][loaded page][new tab page (selected)]. Which I guess could happen if I'm holding ctrl before the click completes.

The about:newtab code has custom click handling code, which presumably is at fault here. It preventDefaults the original click and then does some manual work to record telemetry and/or update frecency or something, and then loads the page by sending a message to the parent. Then code ends up in Redux.jsm and I stop being able to follow what on earth is going on. Maybe Kate can help.

Comment 4

[:Gijs (he/him)]

Note that the remote-page-manager based logic that sends the message from the child knows perfectly well which browser the message came from, but seems to not pass that information to anything it calls after that, which presumably is where the problem comes from...

Comment 5

[Ed Lee :Mardak]

The link click eventually calls openLinkIn

https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/browser/components/newtab/lib/PlacesFeed.jsm#313

Assuming this is a plain link click, where should be "current", and I don't see a params.targetBrowser passed in, so it ends up defaulting to the selectedBrowser:

https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/browser/base/content/utilityOverlay.js#560

Comment 6

[Ed Lee :Mardak]

Oh and to confirm comment 4, yes it does know what tab's browser triggered the message:

https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/browser/components/newtab/lib/PlacesFeed.jsm#312

Comment 7

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

The priority flag is not set for this bug.
:thecount, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[:Gijs (he/him)]

(In reply to Ed Lee :Mardak from comment #6)

Oh and to confirm comment 4, yes it does know what tab's browser triggered the message:

https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/browser/components/newtab/lib/PlacesFeed.jsm#312

Oh, I missed this comment somehow. That makes this easy to fix. Patch coming up.

Comment 9

[:Gijs (he/him)]

Notes:

https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/browser/base/content/utilityOverlay.js#424
https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/browser/base/content/utilityOverlay.js#560

... though interestingly, it looks like if where is "tab", openLinkIn will open the link in the topmost window, rather than in a tab in the window to which targetBrowser belongs... I'm not 100% sure if that's intentional. I expect most callers that pass a targetBrowser would expect "tab" to then open a "sibling" browser in the same window, rather than in the topmost window... Dão, does that sound right?

Comment 10

[:Gijs (he/him)]

Attachment: Bug 1598212 - fix browser selection logic for opening new tab tiles in the same tab, r?Mardak

There's a race condition where we decide we want to open a link in the 'current'
tab, but that tab has changed between when the user clicked the link, and when
we're processing the event. However, the event still knows where it came from,
and we can pass that along to 'openLinkIn' to ensure we open the link in the
correct window/tab.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64909b5afe4d
fix browser selection logic for opening new tab tiles in the same tab, r=Mardak

Comment 12

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/64909b5afe4d

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta for Fx72?

Comment 14

[:Gijs (he/him)]

Comment on attachment 9114142 [details]
Bug 1598212 - fix browser selection logic for opening new tab tiles in the same tab, r?Mardak

Beta/Release Uplift Approval Request

• User impact if declined: race condition in where links opened from about:newtab go
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: n/a
• List of other uplifts needed: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial change ensuring we open about:newtab links from the right context
• String changes made/needed: none

Comment 15

[:Gijs (he/him)]

Alice, if you have a moment, could you confirm this is fixed now?

Comment 16

[Alice0775 White]

(In reply to :Gijs (he/him) from comment #15)

Alice, if you have a moment, could you confirm this is fixed now?

I can not reproduce the issue anymore on Latest Nightly73.0a1(Build ID 20191210095443).

Comment 17

[:Gijs (he/him)]

Verifying per comment #16. Thanks Alice!

Comment 18

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #9)

Notes:

https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/browser/base/content/utilityOverlay.js#424
https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/browser/base/content/utilityOverlay.js#560

... though interestingly, it looks like if where is "tab", openLinkIn will open the link in the topmost window, rather than in a tab in the window to which targetBrowser belongs... I'm not 100% sure if that's intentional. I expect most callers that pass a targetBrowser would expect "tab" to then open a "sibling" browser in the same window, rather than in the topmost window... Dão, does that sound right?

targetBrowser is supposed to be looked at only for the "current" case. We could theoretically do what you suggest for "tab" but targetBrowser would be misleadingly named in that context. I'm also not sure if what you suggest would be better from a users perspective, or if this case even happens at all in the real world.

Comment 20

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9114142 [details]
Bug 1598212 - fix browser selection logic for opening new tab tiles in the same tab, r?Mardak

fix for about:newtab, verified in nightly, approved for 72.0b6

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5c1067881ad1

Comment 22

[Simona Badau, Desktop QA]

I tried to verify this issue on the latest Firefox 72 beta 9 but I could not reproduce it - tested on Ubuntu 18.04 x64 using Firefox 71.

Alice, if time permits, could you please confirm this is also fixed on Firefox 72 beta?

Thanks!

Comment 23

[Alice0775 White]

(In reply to Simona Badau from comment #22)

I tried to verify this issue on the latest Firefox 72 beta 9 but I could not reproduce it - tested on Ubuntu 18.04 x64 using Firefox 71.

Alice, if time permits, could you please confirm this is also fixed on Firefox 72 beta?

Thanks!

I reproduced the issue with Nightly73.0a1(20191206214833).
And I verified the fix with Nightly73.0a1(20191222215627). The issue is no longer reproduced.

And also I can not reproduce the issue with Firefox72.0b10.