Weave should not sync during session restore

RESOLVED FIXED in 1.2

Status

Cloud Services
Firefox Sync: Backend
--
major
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mconnor, Assigned: Mardak)

Tracking

unspecified
Points:
---
Bug Flags:
blocking-weave1.2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
6.74 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
http://mxr.mozilla.org/labs-central/source/weave/source/modules/service.js#255 is killing people with really large sessions.

I don't think there's a true "all tabs are done restoring" notification (there's sessionstore done, but that just means sessionstore has created all the windows/tabs, IIRC), and I don't think loading tabs disturb the idle service, so we should just leave this unbound.  In all but the most ridiculous corner cases, this is perfectly fine.
Flags: blocking-weave1.2+
(In reply to comment #0)
> http://mxr.mozilla.org/labs-central/source/weave/source/modules/service.js#255
> is killing people with really large sessions.
> 
> I don't think there's a true "all tabs are done restoring" notification
> (there's sessionstore done, but that just means sessionstore has created all
> the windows/tabs, IIRC)

You're right, on both accounts. I've been thinking of adding an "actually done restoring" notification. I'm already doing something similar in the profiling work I've started there. Regardless, it wouldn't be in before 3.7 anyway.

So we can use sessionstore-windows-restored on Firefox and stick to the delay on Fennec.
(Reporter)

Comment 2

8 years ago
sessionstore-windows-restored doesn't fire in all cases, it seems (i.e. no windows open on quit (Mac)).  Is there an API I can check to see if there will be a session restored?
(In reply to comment #2)
> sessionstore-windows-restored doesn't fire in all cases, it seems (i.e. no
> windows open on quit (Mac)).  Is there an API I can check to see if there will
> be a session restored?

SessionStartup.doRestore()
(Reporter)

Comment 4

8 years ago
doRestore() will return true, but sessionstore-windows-restored won't fire if there were no windows open.  That's fine, can work around this.  Patch upcoming.
(Reporter)

Comment 5

8 years ago
Created attachment 435973 [details] [diff] [review]
v1

* Fennec does the same thing as before
* Firefox will use a 5 second delay if there's no session to restore
* If there's a session, we'll wait for sessionstore-windows-restored to fire before counting tabs to calculate delay.  We'll also have a 15 second timer to catch cases where sessionstore blows up (we'll update the timer on s-w-r)
Attachment #435973 - Flags: review?(edilee)
(Reporter)

Updated

8 years ago
Whiteboard: [has patch][needs review Mardak]
(Assignee)

Comment 6

8 years ago
Comment on attachment 435973 [details] [diff] [review]
v1

All of these delays are getting pretty complicated.

There's the components file that waits for app-startup which then watches for final-ui-startup which then delays 10 seconds before loading service.js. When this file is loaded, it additionally waits several seconds before actually calling _onStartup from onStartup.

The patch makes the _onStartup delay wait somewhat-conditional but fixes up the timer if sessionstore-windows-restored is triggered. If that event is supposed to fire, it'll definitely fire within the 15 seconds? If not, there may be a duplicate timer set after the original 15-second one triggers.

Also, what's causing the slowdown during sessionstore? Is it actually syncing? Or is it just doing network activity like logging in? Should it just be autologin that gets delayed and not the rest of the service? The longer the service is delayed, the more events each engine/tracker misses out on.
Attachment #435973 - Flags: review?(edilee) → review-
(Reporter)

Comment 7

8 years ago
(In reply to comment #6)
> (From update of attachment 435973 [details] [diff] [review])
> All of these delays are getting pretty complicated.

Yep.  If only they all worked.

> There's the components file that waits for app-startup which then watches for
> final-ui-startup which then delays 10 seconds before loading service.js. When
> this file is loaded, it additionally waits several seconds before actually
> calling _onStartup from onStartup.

This doesn't happen in the general case, load-weave.js loads service.js, so this timer is basically useless.

> The patch makes the _onStartup delay wait somewhat-conditional but fixes up the
> timer if sessionstore-windows-restored is triggered. If that event is supposed
> to fire, it'll definitely fire within the 15 seconds? If not, there may be a
> duplicate timer set after the original 15-second one triggers.

Yeah, that's true.  Can make this smarter.

> Also, what's causing the slowdown during sessionstore? Is it actually syncing?
> Or is it just doing network activity like logging in? Should it just be
> autologin that gets delayed and not the rest of the service? The longer the
> service is delayed, the more events each engine/tracker misses out on.

We can move the autoconnect, not init, that's fine.  We should have done that in the first place, really.  It's still firing before sessionstore fires, so regardless of number of windows opened we never delay more than 5 seconds.
(Assignee)

Comment 8

8 years ago
So then the plan is to init/_onStartup/_checkCrypto when ideal (session store notification with fail-safe) only if this.enabled is still false or maybe Status.service == delayed. Then count the number of busy tabs and schedule autoconnect accordingly.
(Assignee)

Comment 9

8 years ago
Created attachment 436204 [details] [diff] [review]
v2
Assignee: mconnor → edilee
Attachment #435973 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #436204 - Flags: review?(mconnor)
(Assignee)

Updated

8 years ago
Whiteboard: [has patch][needs review Mardak] → [has patch][needs review mconnor]
(Assignee)

Updated

8 years ago
Target Milestone: --- → 1.2
(Reporter)

Comment 10

8 years ago
Comment on attachment 436204 [details] [diff] [review]
v2

yeah, I like this a lot better (even more that when we discussed on IRC!)
Attachment #436204 - Flags: review?(mconnor) → review+
(Assignee)

Comment 11

8 years ago
http://hg.mozilla.org/labs/weave/rev/8f16e8fba53c
Get rid of STATUS_DELAYED and initialize Weave listeners, etc immediately. At the end of onStartup, wait a little bit to let sessionstore restore tabs and then count how many busy tabs to delay autoconnecting to avoid doing network while tabs are doing network.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review mconnor]
You need to log in before you can comment on or make changes to this bug.