Closed Bug 1321119 Opened 3 years ago Closed 3 years ago

Size of synced tab history and assumptions about max record size mean less tabs are synced than otherwise could be.

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: glasserc, Assigned: eoger)

References

Details

Attachments

(1 file)

Yesterday, I accidentally deleted my Firefox profile, because I didn't realize that the profile manager that comes up when you do ./mach run -P is actually the one used globally rather than one specific to the build. Then I continued to use the profile for ~24 hours until the Linux kernel killed it because the machine was out of memory, presumably due to other reasons.

Then I noticed that my profile was gone, and was very sad. But I thought I might be able to check my other computer for any "synced tabs". A lot of them are there, but some are definitely missing. In particular, all the tabs I had open to plan my imminent trip to Hawaii are gone. I believe, although I cannot prove it, that the tabs that were synced were the ones that had been "refreshed" since the previous crash, and that tabs which had been restored but hadn't been "refreshed" are totally gone.

It's possible that my specific use case is unique due to actually deleting a profile rather than merely keeping a bunch of tabs around, but I thought I would open a bug anyhow.
We should dig a little more into this, but I did a very quick experiment:
* Set a profile to restore the session on startup.
* Restart the profile and perform a sync.
* Verified in the sync log that all tabs shown in the profile were synced correctly.
* Verified after the sync that selecting the tabs did cause them to be loaded (ie, that they were not loaded when the sync was done)

and it worked correctly. It sounds like we need to try and reproduce what Ethan saw.
Is this also something that could have changed since 49?
(In reply to Ethan Glasser-Camp (:glasserc) from comment #2)
> Is this also something that could have changed since 49?

Hey Rob - does this ring any bells for you?
Flags: needinfo?(rhelmer)
(In reply to Mark Hammond [:markh] from comment #3)
> Hey Rob - does this ring any bells for you?

oh dear - wrong bug - sorry about that :)
Flags: needinfo?(rhelmer)
(In reply to Ethan Glasser-Camp (:glasserc) from comment #2)
> Is this also something that could have changed since 49?

The only thing I can think of is bug 1250085, but that's 47 :(
I'm pretty sure I've had this bug before version 50, and only on my Windows 10 machine. Older tabs that haven't been refreshed in a while seem to not be synced; manually refreshing them seems to include them in the next sync.

I'm happy to help with testing this bug, and would appreciate guidance on what I can do.
(In reply to juliusp from comment #6)
> I'm happy to help with testing this bug, and would appreciate guidance on
> what I can do.

Thanks! A first good step would be helping to work out how we can reproduce it. If you are familiar with the "browser console", then you can see what tabs Sync will write to the server by executing:

> JSON.stringify(Weave.Service.engineManager.get("tabs")._store.getAllTabs(), undefined, 2)

(which will dump lots of output and be truncated by default - be sure to click the ellipses in the console to see all the output). Note also that this will only work when Sync has been initialized - roughly 10 seconds after startup or immediately after clicking the "sync now" button.

If I restart my browser (which restores the session) and execute this, I see every tab listed. If I then click on one of the tabs, it is clear the tab isn't already loaded - IOW, for me, I see tabs which are yet to be loaded listed.

It would be great if you could verify what this says for you. If some tabs are missing (but not all), we might need to try and deduce what is special about the missing tabs. If no tabs are listed, then I'll be quite confused :)


I think a firs
Ethan, can you please help diagnose this using the steps in comment 7?
Flags: needinfo?(eglassercamp)
Opening the browser console and launching the command given in comment 7 shows all the tabs in the browser, even those not loaded yet. However, going to my personal computer, the Synced Tabs menu doesn't show all of them. As an example, I had a tab on a Bandcamp page and another on the @mozillatech Twitter account. Both of these showed up in output of the command from comment 7. These tabs did not show up in the Synced Tabs menu on my personal computer, nor did they show up as a synced tab when I typed info relevant to them in the awesomebar. (They show up, but not as "Ethan's Firefox on [work computer]".) However, once I click these tabs, which causes them to refresh, and then explicitly sync first the work computer and then the personal computer, they show up as normal synced tabs.
Flags: needinfo?(eglassercamp)
For reference, here's also the text representing those tabs in the output of the command:

  {
    "title": "knife city",
    "urlHistory": [
      "https://knifecity.bandcamp.com/",
      "resource://activity-streams/data/content/activity-streams.html#/"
    ],
    "icon": "https://f4.bcbits.com/img/a0743268791_3.jpg",
    "lastUsed": 1480728189
  },
  {
    "title": "Mozilla Tech (@mozillatech) | Twitter",
    "urlHistory": [
      "https://twitter.com/mozillatech"
    ],
    "icon": "https://abs.twimg.com/favicons/favicon.ico",
    "lastUsed": 1480728190
  },
Hi everyone,

FYI, continuing to follow this--unfortunately, my Windows machine has just died (great timing), so I'm unable to help at the moment. When I'm back up and running, I'll check in again to see how I can help things along.

--Julius
I'll investigate this further.
Assignee: nobody → markh
Priority: -- → P1
Assignee: markh → kit
Assignee: kit → markh
I think comment 9 is just describing sync latency. In the general case, opening new tabs doesn't force an immediate sync (as otherwise we'd be constantly syncing for many users). Sync typically runs ever 10 minutes, so it's certainly possible that tabs opened in the last 10 minutes (or 10 minutes before Firefox was shut down) aren't going to be in the list of tabs from that computer.

Then we have the second device that is displaying these synced tabs - it too needs to have recently synced to see the most recent tabs on all other devices. When we show the Synced Tabs menu, we typically show the list of tabs from the most recent sync, but immediately start a sync of tabs in the background. Once this completes we refresh the list. In other words, when you open Synced Tabs, you are likely to initially see the tabs fetched 10 minutes ago, before it fairly quickly changes to the most recent set.

ISTM that comment 9 could be explained by the above. It also verifies that the list of tabs that sync builds does include these yet-to-be-loaded tabs.

Julius, you mentioned you have seen this - are you able to try and help reproduce this? In particular, try reproducing an issue even after explicitly syncing the initial device (to ensure all tabs currently open should be uploaded to the servers), then viewing the list of tabs on the second device and see if any are missing?

Sadly, if there's no problem in that scenario, we will probably mark this as WONTFIX - syncing far more often just to avoid this issue will probably put our server infrastructure under more load than is reasonable.

Thanks!
Flags: needinfo?(juliusp)
When you say "new tabs", do you mean "tabs that have been around for weeks but haven't been viewed since Firefox restarted"?

I don't understand how this could be related to latency. I just did a sync on my "work" computer and my "home" computer. On my "work" computer, the command:

> Weave.Service.engineManager.get("tabs")._store.getAllTabs().length

prints 105. On my "home" computer, under "Synced tabs" for my "work" computer, I see ~30ish tabs (counted by hand).
Flags: needinfo?(juliusp)
(In reply to Ethan Glasser-Camp (:glasserc) from comment #14)
> When you say "new tabs", do you mean "tabs that have been around for weeks
> but haven't been viewed since Firefox restarted"?
> 
> I don't understand how this could be related to latency. I just did a sync
> on my "work" computer and my "home" computer. On my "work" computer, the
> command:

Sorry, I misunderstood your comment.

> > Weave.Service.engineManager.get("tabs")._store.getAllTabs().length
> 
> prints 105. On my "home" computer, under "Synced tabs" for my "work"
> computer, I see ~30ish tabs (counted by hand).

Yeah, that sounds like bug 1244622 - we only show the 50 "most recent" tabs. I guess it would be interesting to know if it really is only 30 you see - it's certainly possible there is another issue hiding in here that's not covered by any of the above speculation.
Well, Firefox crashed on my work computer last night, and I restarted it as normal. This means there is a large quantity of "unrefreshed" tabs. On my home computer, the "Synced Tabs" menu list for my work computer has 21 tabs. I assumed at first that this was just the number of tabs that had been refreshed, but I see that some of the refreshed tabs weren't on the list. I closed a couple of refreshed tabs that WERE on the list and repeated the sync, and the tabs that I closed were replaced by refreshed tabs that hadn't been on the list. I flipped through some tabs that seemed refreshed and closed a couple more, and somehow I got up to 24 tabs on the list. However, I pulled up an unrefreshed tab, watched it refresh, and did another sync, and now it's 25 (with the newly-refreshed one on the list). I keep refreshing and closing tabs and I just saw a tab that wasn't refreshed show up on the list and watched that tab refresh when I clicked on it. However, those unrefreshed tabs that show up on the list seem to be adjacent to refreshed ones, so maybe it's pre-refreshing them because I was nearby?
Thanks for continuing to help us here!

(In reply to Ethan Glasser-Camp (:glasserc) from comment #16)
> Well, Firefox crashed on my work computer last night, and I restarted it as
> normal. This means there is a large quantity of "unrefreshed" tabs.

To be clear, this should be the exact same scenario as a simple restart and "restore previous session", right?

> However,
> I pulled up an unrefreshed tab, watched it refresh, and did another sync,
> and now it's 25 (with the newly-refreshed one on the list). I keep
> refreshing and closing tabs and I just saw a tab that wasn't refreshed show
> up on the list and watched that tab refresh when I clicked on it. However,
> those unrefreshed tabs that show up on the list seem to be adjacent to
> refreshed ones, so maybe it's pre-refreshing them because I was nearby?

That's really odd, and no matter what I try, I can't reproduce any weirdness here :(

If I use the https://github.com/mhammond/aboutsync addon, I *always* see the number of tabs for the device reflecting what tabs are actually open from the device, and the synced tabs menu always shows that exact list of tabs - even after killing the process for the profile, restarting the profile, immediately syncing before selecting any tabs and immediately exiting.

If you can reproduce this reliably (eg, by restarting and restoring a session) it would be great if you could use that addon to try and see what it going wrong (ie, manually checking the count of tabs there reflects both the other device and what the menu shows).

I'm going to unassign myself here and flag this as needing STR, but I'll certainly jump back on it as soon as any new information comes to light.
Assignee: markh → nobody
Priority: P1 → P3
I was able to reproduce *something*. I'm not sure that what I produced was exactly the same thing that I encountered in situ, but it seems similar so here you are. I also was able to somehow cause multiple records to be uploaded to the "tabs" collection, but I don't know exactly how I was able to do that.

This was against an official Nightly build.

1. ./firefox --no-remote -P. Create a profile called "create-missing-tabs" and launch it.
2. Set up some aids. Menu -> Preferences -> When Nightly starts, show my tabs and windows from last time. Open dev tools and "Enable browser chrome and add-on debugging toolboxes". Install "About Sync" add-on.
3. Open a bunch of tabs. I suggest https://en.wikipedia.org/wiki/January_1 and then ctrl-clicking every day from January, February, and March.
4. Log into sync. I created a new account called "myusername+toomanytabscreate@mozilla.com". I used the confirmation link by opening the email in another profile, right clicking the link and choosing "Copy link location", and pasting it to the create-missing-tabs profile.
5. Wait for syncing to finish.
6. Open "About Sync". Observe under "tabs" that there is one record, which shows 92 tabs (the 91 from January, February, and March, plus the "Account verified" one). So far, so good!
7. Ctrl-Q out. Restart.
8. Open a new tab. Navigate to https://fr.wikipedia.org/wiki/1er_janvier and ctrl-click the days in January. DO NOT SYNC. Before a sync can happen organically, Control-C in the terminal. Restart.
9. Force a sync. Load About Sync. Observe that under "tabs" there is still one record, which shows 105 tabs. However, it should show closer to 123 tabs. (92 from before, plus the French January 1-31. However, when I restarted, I saw that I only had January 1-29. I guess I failed to click the last two, or maybe they didn't make it into the session before I ctrl-c'd the thing.)
10. Open the browser console and execute `Weave.Service.engineManager.get("tabs")._store.getAllTabs().length`. Verify that this doesn't show 105, but something closer to 123.

The ones that don't seem to have made it to Sync are the English January 1-17, i.e. the first ones to be opened.

Now, if this were exactly the same as the issue I described in comment 0, then we'd see much fewer tabs, for example as few as 1 (since we do a sync with only one tab "refreshed"). I wasn't able to reproduce exactly that in just a couple hours, but I thought I would start by submitting this since it demonstrates a disparity between the Weave command in the browser and what gets sent to Sync. Maybe each time it crashes, it loses a few more tabs. Or maybe once there's a disparity, tabs just fall off the back of the list or something.
I can reproduce this. However, there are 2 things I forgot to mention (mainly because I forgot it existed) about our implementation:

* At https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/services/sync/modules/engines/tabs.js#212 we have special logic to drop the least-recently used tabs so that we don't exceed the maximum payload size of the "tabs" record. The logs show we hit that:

> 1487220473329	Sync.Store.Tabs	TRACE	Created tabs 113 of 118

(ie, 5 tabs were dropped when I did this.

* getAllTabs() has a "filter" param which defaults to false, whereas when we build the tab list to sync we specify true. IOW, when I did it, I see |Weave.Service.engineManager.get("tabs")._store.getAllTabs().length| return 120, whereas |Weave.Service.engineManager.get("tabs")._store.getAllTabs(true).length| returns 118. The 2 excluded tabs are a chrome:// URL (but about:, file: etc pages are also filtered)

I *hope* this accounts for most of the above issues.

However, that said, there is still a number of things we can do here to improve this significantly:

* Reduce the size of each tab: The size of each tab varies (obviously due to the title, but less obviously because we also include up to 25 entries of "history" for each tab) it's difficult to predict what that number would be. However, we don't ever *use* those 25 history items - when we open the remote tab we just open the URL itself and don't attempt to pretend there are 25 items in the "back" button. I've never been comfortable suggesting we implement that a change to sessionstore could mean we end up with data we don't understand.

* Handle more tab: The code that tries to ensure the payload can fit is from bug 535326, when the max payload size for a record was apparently 28k - which is hard-coded into that function. Apparently it is currently 262144 bytes - so in theory we could store ~10x the number of tabs (and note that recently the server actually tells us the max payload size, so we can avoid the hard-coding)

I think we should probably consider doing both of these things:

* change createRecord to use the server-supplied max_record_payload_bytes value, possibly sticking with the current 28k assumption if we somehow fail to know what that value is.
* optionally only store the most recent history entry (ie, only store the URL of the current tab, not the "back" history. Or maybe only 5 or something. (I say "optionally" because I think the above will solve the problem for most users who currently hit it)

That should then make this issue be almost impossible to hit in practice.

I'm going to repurpose this bug to do that - thanks for your persistence in the face of my denials that anything is wrong ;)
Priority: P3 → --
Summary: Tabs from a restored session that haven't been refreshed are not synced → Size of synced tab history and assumptions about max record size mean less tabs are synced than otherwise could be.
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8842579 [details]
Bug 1321119 - Allow to Sync more tabs and reduce tabs history to 5.

https://reviewboard.mozilla.org/r/116364/#review118594

Perfect, thanks!
Attachment #8842579 - Flags: review?(markh) → review+
See Also: → 1344127
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/909cf4b97773
Allow to Sync more tabs and reduce tabs history to 5. r=markh
https://hg.mozilla.org/mozilla-central/rev/909cf4b97773
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
QA Contact: sorina.florean
You need to log in before you can comment on or make changes to this bug.