1344127 - tabs engine excludes records > ~1MB, but it should be using ~256kb

Status: RESOLVED WORKSFORME

(Firefox :: Sync, enhancement, P3)

Description

[Mark Hammond [:markh] [:mhammond]]

In bug 1321119 we tweaked the algorithm for how many tabs we can sync based on the maximum record size.

However,  https://dxr.mozilla.org/mozilla-central/rev/d29f84406483c721a13cf9a52936ecced0c5c98a/services/sync/modules/engines/tabs.js#168 has another length check to exclude URLs > MAX_UPLOAD_BYTES, which is defined as |1024 * 1023, // just under 1MB| - however, that's wrong - it really should be checking the max record size (ie, just under ~256kb)

IIUC, the current behaviour of this will be:
* the MAX_UPLOAD_BYTES check will allow a URL > max-record-size.
* the logic touched in 1321119 will then start nuking tabs until the total size goes under the max-record limit.
* if the tab with the huge URL is the most recently used, the end result is that zero tabs will be listed.

If we just changed that MAX_UPLOAD_BYTES check to use the max record size, I believe the worst case would be exactly 1 tab with a very long URL. While that seems somewhat better, I don't think its ideal - an average user would probably prefer to *not* have that long URL but instead every other URL they have open. So it might make sense to have it be something like max-record-size / 10 or so, which would be a URL of roughly 25k - still very long, but short enough it isn't going to exclude all other tabs.

This is really a UX issue, so needinfo rfeeley, and also CC rnewman as he always has opinions on things like this :)

To rephrase. If a user has a tab open with a *very* long URL:

* currently they may see zero tabs from the device. This is clearly bad.

* the "simple" fix to this would be that such users may see exactly 1 tab from that device - the tab with the huge URL.

* I'm suggesting that we change this so that the max size of a URL is ~25KB, meaning the user is guaranteed to see multiple tabs from the device, but any with URLs > 25kb will never appear.

Another way to look at it: a user with just one massive tab open would lose (we will not sync it, whereas in the current world we might). A user with multiple tabs open, where only 1 is massive probably wins - they see their other tabs, but still not the long one)

Another possibility is we change the algorithm so that instead of dropping the least-recent tabs until they can fit, we instead drop the tabs with the longest URL until they fit. That has the risk of surprising a user with many many tabs open but none of them being massive - the tabs we drop will appear arbitrary.

Thoughts?

Comment 1

[Ryan Feeley [:rfeeley]]

Your suggestion to make a max size for the URL makes the best of an unfortunate situation. I approve!

Comment 2

[Richard Newman [:rnewman]]

Options:

- Discard URLs in order until record is small enough.
  - But "order" is kinda meaningless: we sort by access time!
- Discard URLs in order…
  - … and define "order" to be size-wise.
- Filter out URLs that are too long, FSVO "too long"
  - Leaving some arbitrary subset of URLs, and apparent gaps.
- Truncate URLs.

I suspect that truncating URLs to some reasonable-yet-small length -- say, 10,000 chars -- will offer the best result: tabs won't seem to disappear, many truncated URLs will function (perfectly or imperfectly), and payload sizes will still be controlled. If a user has 25 tabs open with long URLs, they'll all be truncated, but all the tabs will be visible. If the truncated URLs don't work, at least the user can see the tab, and find the working URL in history.

(We'll have to truncate titles, too, of course.)

Comment 3

[Mark Hammond [:markh] [:mhammond]]

The new limit is URI_LENGTH_MAX (64k) and I believe we already do the right thing WRT ensuring we can fit tabs, so WFM