Closed Bug 1390478 Opened 3 years ago Closed 3 years ago

menus don't show sync status until too long after startup

Categories

(Firefox :: Sync, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: dietrich, Assigned: markh)

Details

Attachments

(1 file)

57.0a1 (2017-08-15) (64-bit) Mac OS X

STR:

1. start Firefox
2. open the cheeseburger menu

Expected: Says I'm signed into Sync.

Actual: Doesn't. Later it does, though!
More info:

* 347 tabs
* about:support report: https://pastebin.mozilla.org/9029769
How long does it take for you? We recently optimized this for minimal startup cost but it shouldn't take "too long".
Just timed it, about 40 seconds.
We may want to add a 5 or 10 sec timeout in the initialization code [0], what do you think Florian?

[0] http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#1705-1708
Flags: needinfo?(florian)
(In reply to Dietrich Ayala (:dietrich) from comment #3)
> Just timed it, about 40 seconds.

Is it the time it takes for the CPU usage to go down after restoring all your tabs?

(In reply to Edouard Oger [:eoger] from comment #4)
> We may want to add a 5 or 10 sec timeout in the initialization code [0],
> what do you think Florian?
> 
> [0]
> http://searchfox.org/mozilla-central/source/browser/base/content/browser.
> js#1705-1708

That would be ok. But I wonder if we should just force the initialization when someone clicks the hamburger button.
Flags: needinfo?(florian)
So it turns out it is a bit more complicated than this, in UIState.jsm _refreshFxAState does a profile HTTP call.
The UI won't be updated until that HTTP call finishes and I'm guessing this is what happened for :dietrich.

There are some solutions here:
- We can either update the UI 2 times unconditionally (once after reading the FxA data, then after the profile calls eventually succeed)
- We could also put a Promise timeout on that HTTP request (with Promise.race(), 5 sec max for example). If we are out of time, though luck, update the UI and try next time.
- We could implement some kind of hybrid of the two previous solutions: if we get a Promise timeout, update the UI and update the UI when the profile HTTP request is done. It requires more refactoring, and I'm not sure it is really worth it.
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> (In reply to Dietrich Ayala (:dietrich) from comment #3)
> > Just timed it, about 40 seconds.
> 
> Is it the time it takes for the CPU usage to go down after restoring all
> your tabs?

No way! These days it's WAY faster than that :D


(In reply to Edouard Oger [:eoger] from comment #6)
>
> The UI won't be updated until that HTTP call finishes

This seems like a problematic pattern.
(In reply to Edouard Oger [:eoger] from comment #6)
> So it turns out it is a bit more complicated than this, in UIState.jsm
> _refreshFxAState does a profile HTTP call.
> The UI won't be updated until that HTTP call finishes and I'm guessing this
> is what happened for :dietrich.

Don't we cache the profile these days and immediately use the cached version, kicking off a background request to update the profile if it is stale?
I don't think we cache the profile on disk though, so we have to do a request.
I think we do via http://searchfox.org/mozilla-central/source/services/fxaccounts/FxAccounts.jsm#1635 - my signedInUser.json certainly has it.
I have created a Nightly build that logs what is happening when we refresh the sync UI state.
If you can reproduce the problem reliability, could you run the build until the hamburger menu gets updated and copy-paste the content of the browser console somewhere?

Here is a direct link to the build: https://queue.taskcluster.net/v1/task/RzlS6upcRUqQsZC1DIW1Bg/runs/0/artifacts/public/build/target.dmg
from https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c123ee7f8f887eb92b616004273319324391b12
Assignee: nobody → eoger
Priority: -- → P1
Here's an up-to-date build:
https://queue.taskcluster.net/v1/task/IkRK8i-eR8C6dQ3L5jn0_w/runs/0/artifacts/public/build/target.dmg
If you have time to try it Dietrich.
Flags: needinfo?(dietrich)
Deprioritized. Can't work on this until we hear back from reporter.
Priority: P1 → P2
I don't have time to dig into this, and also the test build is no longer there.

But you shouldn't need me in order to fix this.

As long as your code is racing the user vs the http request completing, anyone on a slow network can experience this to varying degrees.

Loads of networking throttling tools are available for creating these conditions.

It also sounds like, per comments #8 - #10, that this is only happening because of maybe some other underlying bug that probably needs to be looked at.
Flags: needinfo?(dietrich)
Assignee: eoger → nobody
Flags: needinfo?(markh)
We do block on the profile request if we don't have a cached copy, which is an obvious improvement we could make. However, it does seem unlikely we'd hit this in practice any time other than the user signing up.
Flags: needinfo?(markh)
Not blocking for the profile is a simple fix.
Assignee: nobody → markh
Comment on attachment 8935580 [details]
Bug 1390478 - never block the UI waiting for the FxA profile.

https://reviewboard.mozilla.org/r/206460/#review212470

LGTM, thanks!
Attachment #8935580 - Flags: review?(eoger) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/0e26ab46cbb7
never block the UI waiting for the FxA profile. r=eoger
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/36187d67ca82
never block the UI waiting for the FxA profile. r=eoger
https://hg.mozilla.org/mozilla-central/rev/36187d67ca82
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Flags: needinfo?(markh)
You need to log in before you can comment on or make changes to this bug.