1794474 - F5 / refresh on firefox_view should request a tab sync if signed in

Status: VERIFIED FIXED

(Firefox :: Firefox View, enhancement, P2)

Description

[Sam Foster [:sfoster] (he/him)]

If firefox-view is showing what the user perceives to be stale synced tab data, their instinct is going to be to hit the refresh button (or F5, or ctrl+R.) As the page represents data and state held in a shared system module this doesn't currently accomplish anything. I think it should request a tab sync if the user is signed-in and has syncing devices. If we're in an error state, refresh should maybe do the same thing the "try again" primary button would normally do to try to clear the error.

Comment 1

[Ray Fambro]

I think this suggestion makes sense Sam. Let me know if this is still feasible to land in 107 given where we are in the timeline.

Comment 2

[Sam Foster [:sfoster] (he/him)]

If you hit f5 on the firefox-view tab, BrowserReloadWithFlags(reloadFlag) gets called. For a regular tab that will end up calling sendReloadMessage, but for the fx-view tab we end up returning early because it is not in gBrowser.selectedTabs, because of the _mayTabBeMultiselected filter.

You can confirm this from the console with the fx-view tab selected by evaluating gBrowser._mayTabBeMultiselected(FirefoxViewHandler.tab) which returns false, and although gBrowser.selectedTab == FirefoxViewHandler.tab, gBrowser.selectedTabs.includes(FirefoxViewHandler.tab) is false.

Comment 3

[Tim Giles [:tgiles]]

:Gijs, what can we do about this situation. Given that the Firefox View tab cannot be multiselected, we don't send a reload message and so the Firefox View tab isn't reloaded. I understand, thanks to :sfoster, why this would be the case. It wouldn't make sense to try and move tabs from one window to another if Firefox View was included in that selection. However, I'm not sure how Firefox View could listen to the "BrowserReloadOrDuplicate(event)" command (or even if that makes any sense).

I tried to go down the docShell.loadType & Ci.nsIDocShell.LOAD_CMD_RELOAD route, but unfortunately that statement always returns 0 in my case. I tried reloading content pages and the bitwise comparison would still return 0. Entirely possible I'm missing something so feel free to fill in the gaps.

If we can get the sendReloadMessage to work as expected on Firefox View, then we should be able to listen/observe the type of the most recent navigation via performance.getEntriesByType('navigation') and checking if the type is "reload" or not thanks to PerformanceNavigationTiming.type. But yeah, that presumes we can get reloading to work as expected for the Firefox View tab. I'm not sure what the least risky play is here, whether to make Firefox View a multiselectable tab or to try and listen to the BrowserReloadOrDuplicate command and make our own Firefox View version of that function that sends the reload message to the actor. Open to suggestions to keep this moving forward.

Comment 4

[:Gijs (he/him)]

At https://searchfox.org/mozilla-central/rev/f118dae98073bc17efb8604a32abfcb7b4e05880/browser/base/content/browser.js#3566 can we sub out

  for (let tab of gBrowser.selectedTabs) {

with

  let tabs = gBrowser.selectedTabs;
  if (!tabs.length) {
    tabs = [gBrowser.selectedTab];
  }
  for (let tab of tabs) {

with a code comment explaining why this is necessary? I think that should fix the issue you describe.

Comment 5

[Tim Giles [:tgiles]]

Thanks :Gijs! Switching that snippet out allows us to reload the Firefox View tab when clicking the Refresh button or when using F5. I'm not seeing any other issues with this change at the moment, but I'm not sure where issues would appear from making this change.

:sfoster, so do we want to clear the "services.sync.lastTabFetch" preference on reload? I tried this and we would still need to wait the 30 seconds as indicated in the syncTabs function. So while we're able to reload the Firefox View page now, the synced tab data could still be stale on page reload. I know the other solution would be to use something similar to TabsSetupFlowManager.tryToClearError() where we manually request a sync. By using this, the tab pickup list UI updates as well (rather quickly but still) giving another visual confirmation that refreshing the Firefox View page actually did something. This could result in a lot of pings to the sync service though which I'm assuming we've talked about before when first implementing the tryToClearError() function (I'm sure I'm missing context).

I'm personally leaning more towards the tryToClearError() approach, I think it's a good idea to show the user that something has happened when refreshing the page but I don't have a strong opinion about either solution.

Comment 6

[:Gijs (he/him)]

We could fix the refresh issue directly in bug 1792680 and mark that as a blocker for this one, maybe?

In terms of where to fix this, I think if we force a sync, but only in response to a reload/refresh, that shouldn't be too bad? I'd prefer not to clear the "last fetch" bit - AIUI if we pass force to the sync tabs method, that will successfully sync anyway. I assume we do something similar when people click the "sync now" button. If we're still worried about this we could potentially make it a no-op for a shorter interval than the default TABS_FRESH_ENOUGH_INTERVAL_SECONDS.

Comment 7

[Sam Foster [:sfoster] (he/him)]

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

In terms of where to fix this, I think if we force a sync, but only in response to a reload/refresh, that shouldn't be too bad? I'd prefer not to clear the "last fetch" bit - AIUI if we pass force to the sync tabs method, that will successfully sync anyway.

Agreed, I suggested clearing that pref assuming we would end up calling SyncedTabs.syncTabs() at some point in there. But I think this is a reasonable moment to force a (tab) sync. (i.e. SyncedTabs.syncTabs(true))

Comment 8

[Tim Giles [:tgiles]]

Hmm, I'm stuck on a new issue now. So I thought I would be able to use performance.getEntriesByType("navigation") and just check if the navigation entry type was "reload" or not but it seems like once Firefox View is in a "reload" state, it will never change back to "navigate". I thought that backgrounding the Firefox View tab and then foregrounding the tab would cause a "navigate" event and reset the navigation state. In other words, once you reload the Firefox View tab, it stays reloaded forever it seems. Normally this wouldn't be a big deal, but this means that every time you restart that Firefox instance and then go to Firefox View, we'll force a sync. This is because I'm currently triggering the forced sync on DOMContentLoaded after the MediaQueryDOMSorting function is called. For some reason, when you navigate to Firefox View after restarting the browser, it takes a noticeable amount of time to sync (like 15 seconds or something) compared to the almost instantaneous current implementation. Obviously a delay that long is unacceptable especially given the current implementation and so I'm not sure what to do next. Strangely enough, any sync after the first is almost instantaneous and I'm not entirely sure why. Maybe it has something to do with me making the DOMContentLoaded callback async to handle the SyncTabs function call?

Hmm, actually it always seems like the initial sync when starting the browser takes a noticeable amount of time. Minus my async changes and what not, I'm able to get into the "Sit tight while your tabs sync" for more than 30 seconds if I just wait on the Firefox View page. Not sure if that's a known issue or not, or maybe it's Sync throttling me as I've been sending a lot of "sync tabs now" request when writing this patch. Given that I can reproduce this on a recent central commit, I'm going to assume my changes are not causing this initial tab loading issue. Because of that, I think my previous concern about "async DOMContentLoaded callback is the issue" is no longer valid and I can keep going with that approach.

Now to figure out how to test this bug.

Comment 9

[Sam Foster [:sfoster] (he/him)]

(In reply to Tim Giles [:tgiles] from comment #8)

Hmm, actually it always seems like the initial sync when starting the browser takes a noticeable amount of time. Minus my async changes and what not, I'm able to get into the "Sit tight while your tabs sync" for more than 30 seconds if I just wait on the Firefox View page.

This sounds like bug 1792040 - or at least is something I'm running into and hoping to fix in the patch on that bug.

We do need to be careful we don't force a tabSync more than once in a given timeframe, so its important we can clearly distinguish between a first-load of firefox-view vs. a tab-select vs. a user-triggered reload.

Comment 10

[Sam Foster [:sfoster] (he/him)]

(In reply to Tim Giles [:tgiles] from comment #8)

Strangely enough, any sync after the first is almost instantaneous and I'm not entirely sure why.

Not strange, we just cache synced tabs so the first time we have to wait for the sync to finish to have anything to show, and every time afterwards we show what we have and update it later when the sync finishes.

Comment 11

[Tim Giles [:tgiles]]

Attachment: Bug 1794474 - Force tabs sync when a user reloads Firefox View. r=sfoster

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81721704c6cc
Force tabs sync when a user reloads Firefox View. r=sfoster

Comment 13

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/81721704c6cc

Comment 14

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9298652 [details]
Bug 1794474 - Force tabs sync when a user reloads Firefox View. r=sfoster

Beta/Release Uplift Approval Request

• User impact if declined: Its possible to get stuck in a loading/waiting state in firefox view, or to be presented with stale synced tabs while waiting for the next sync.
• 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:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Patch is confined to front-end firefox view code, and adds reload/F5 handling without touching other functionality.
• String changes made/needed: None
• Is Android affected?: No

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9298652 [details]
Bug 1794474 - Force tabs sync when a user reloads Firefox View. r=sfoster

Approved for 107.0b3.

Comment 16

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e94233fc1189

Comment 17

[Rares Doghi, Desktop QA]

Verified as fixed in our latest Beta 107.0b3 as well as our latest Nightly Build.

Comment 18

[Rares Doghi, Desktop QA]

Hi Tim, will this fix also reach our Release 106 ? or should someone mark it with Wont fix ?

Comment 19

[:Gijs (he/him)]

If we wanted to take this on 106 for the upcoming planned dot release we would also need to take bug 1792680 which relman marked wontfix. Unless Pascal disagrees, I think we could potentially take both, but before we do it would be nice to resolve the confusion in bug 1796685 to ensure we're not somehow regressing SVG performance with this change (to be clear, I very much doubt that we are, but that's what that bug says right now).

Comment 20

[Pascal Chevrel:pascalc (PTO until April 26)]

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

If we wanted to take this on 106 for the upcoming planned dot release we would also need to take bug 1792680 which relman marked wontfix. Unless Pascal disagrees, I think we could potentially take both, but before we do it would be nice to resolve the confusion in bug 1796685 to ensure we're not somehow regressing SVG performance with this change (to be clear, I very much doubt that we are, but that's what that bug says right now).

I have no objection to taking this fix if we are confident that the regression is not a problem. Note that we are advancing our planned dot release to end of this week so as to get a crash fix out of the door faster so I would need the uplift requests and a confirmation on the SVG status tomorrow. Thanks.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Pascal Chevrel:pascalc from comment #20)

I would need the uplift requests and a confirmation on the SVG status tomorrow. Thanks.

Regarding the SVG thing - as one of the "owners" for the SVG perf metric in question, I'm pretty confident that the perf regression here (bug 1796685) is just noise. Or to the extent it's real, it almost certainly was caused by a different bug in the "guilty" push range.

So: no concerns there for uplift impact.

Comment 22

[Sam Foster [:sfoster] (he/him)]

Attachment: Patch for 106: bug-1794474-106.patch

Approval Request Comment
[Feature/Bug causing the regression]: Firefox View (new feature in 106)
[User impact if declined]: Its possible to get stuck in a loading/waiting state in firefox view, or to be presented with stale synced tabs while waiting for the next sync.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: 1792680
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Patch is confined to front-end firefox view code, and adds reload/F5 handling without touching other functionality.
[String changes made/needed]: None

The 107 patch has a conflict when graft to release/106 because of the MediaQueryDOMSorting.init(); line we've added since. This patch is the result of manually fixing that conflict.

I'm clearing :Gijs' and tgiles' need-infos as dholbert has answered the question about the SVG regression in Comment 21. And I'm answering the question about 106 uplift here - if possible, yes please.

Comment 23

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9300037 [details] [diff] [review]
Patch for 106: bug-1794474-106.patch

Approved for our upcoming dot release, thanks.

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-release/rev/cb57c0c3b0eb

Comment 25

[Rares Doghi, Desktop QA]

Verified as fixed in our latest Release 106.0.2.