535326 - Need to rethink tab sync limit of 25

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Ragavan S [:rags]]

So, we currently have an arbitrary limit of syncing only 25 of your "recent" tabs. This feels broken. 

I'm opening this bug so we can track this issue and come up with a better UX.

Comment 1

[Klark Fusco]

I came here looking specifically for this issue and was glad to see a bug had been opened. 25 tabs does seem arbitrary and for me at least too low. Couldn't this just be configurable somewhere, or set much higher at the very least? Right now it just looks like the sync failed after 25 tabs.

Comment 2

[Mike Connor [:mconnor]]

It's a semi-arbitrary limit, set this low to avoid overloading a) the UI and b) the max record size, which was causing full-out failures.  I'd like to implement something better for the UI, and then fix the sync engine to remove the limits on number of tabs synced (or, at least increase this significantly).

Comment 3

[Martin Mueller]

(In reply to comment #1)
> I came here looking specifically for this issue and was glad to see a bug had
> been opened. 25 tabs does seem arbitrary and for me at least too low. Couldn't
> this just be configurable somewhere, or set much higher at the very least?
> Right now it just looks like the sync failed after 25 tabs.

It would be very handy, to set up a personal limit of tab-sync. 25 tabs are too few for a workday. More would be helpful.

Comment 4

[Mike Connor [:mconnor]]

The main problem here is that setting the limit too high will lead to failures at a non-deterministic number of tabs, meaning the machine will fail to sync.  bug 542315 will fix that.

Mardak, any ideas on how to derive a reliable maximum number of tabs we can sync in the meantime?

Comment 5

[Ed Lee :Mardak]

We should know what's the maximum record size and some overhead on the record from metadata/encryption, so we could potentially fill up the payload to a certain string length instead of a fixed number.

Comment 6

[Mike Connor [:mconnor]]

We could do that, yeah, though I don't think the encryption overhead is deterministic enough to do it without a pretty significant fuzz factor.  Will play with that idea tomorrow!

Comment 7

[Mike Connor [:mconnor]]

I never played with that idea, fail.  Punting to Ed to load balance a little, basically want to a) make this a pref and b) figure out how high we can bump this.

Comment 8

[Ed Lee :Mardak]

The contents of an encrypted payload consists of {"encryption":"https://.....", "ciphertext":"..."}

The encryption string plus other JSON overhead will be about 80 in our case, but it depends on the server url and username.

The ciphertext will be of lengths that are multiples of the cipher block size, and additionally the ciphertext is base64 encoded, so 4/3.. 1.33% overhead.

The maximum payload size accepted by the server right now seems to be 28KB (28672B). So /4*3 = 21504 then subtract off various JSON/padding/cryptowrapper overheads and the initial payload can be up to 21000 bytes with 500 bytes of leeway.

Comment 9

[Ed Lee :Mardak]

Attachment: v1

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 427510 [details] [diff] [review]
v1

r=me, though I wonder if we want more bulletproofing against too-large payloads (I don't know how consistent the encryption overhead will be, so in theory this could still fail.

Comment 11

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/94a0edeb8883
Fit as many tabs as possible in 20000 characters by linearly estimating how many will fit then remove extras one by one.

Comment 12

[Tony Chung [:tchung]]

Not sure how to best test this sanely by hand.  Sounds like something for a unit test to check the boundaries

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #12)
> Not sure how to best test this sanely by hand.  Sounds like something for a
> unit test to check the boundaries

Probably the best way is to open 26 tabs and see if they all get synced. 26 should fit in 20000 characters since we're not syncing any tab history at the moment.

Comment 14

[Tony Chung [:tchung]]

Verified fix on weave 1.2 across fennec and firefox 3.6.3

Comment 15

[Jonathan Griffin (:jgriffin)]

Crossweave test added