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

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Dietrich Ayala (:dietrich)]

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!

Comment 1

[Dietrich Ayala (:dietrich)]

More info:

* 347 tabs
* about:support report: https://pastebin.mozilla.org/9029769

Comment 2

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

How long does it take for you? We recently optimized this for minimal startup cost but it shouldn't take "too long".

Comment 3

[Dietrich Ayala (:dietrich)]

Just timed it, about 40 seconds.

Comment 4

[Edouard Oger [:eoger]]

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

Comment 5

[Florian Quèze [: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.

Comment 6

[Edouard Oger [:eoger]]

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.

Comment 7

[Dietrich Ayala (:dietrich)]

(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.

Comment 8

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

(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?

Comment 9

[Edouard Oger [:eoger]]

I don't think we cache the profile on disk though, so we have to do a request.

Comment 10

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

I think we do via http://searchfox.org/mozilla-central/source/services/fxaccounts/FxAccounts.jsm#1635 - my signedInUser.json certainly has it.

Comment 11

[Edouard Oger [:eoger]]

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

Comment 12

[Edouard Oger [:eoger]]

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.

Comment 13

[Julie McCracken (:julie)]

Deprioritized. Can't work on this until we hear back from reporter.

Comment 14

[Dietrich Ayala (:dietrich)]

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.

Comment 15

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

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.

Comment 16

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

Not blocking for the profile is a simple fix.

Comment 17

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

Attachment: Bug 1390478 - never block the UI waiting for the FxA profile.

Review commit: https://reviewboard.mozilla.org/r/206460/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206460/

Comment 18

[Edouard Oger [:eoger]]

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!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e26ab46cbb7
never block the UI waiting for the FxA profile. r=eoger

Comment 20

[Dorel Luca [:dluca]]

Backed out changeset 0e26ab46cbb7 (bug 1390478) for Xpcshell failure in services/fxaccounts/tests/xpcshell/test_profile.js on a CLOSED TREE

https://treeherder.mozilla.org/logviewer.html#?job_id=152345824&repo=autoland&lineNumber=7994

https://hg.mozilla.org/integration/autoland/rev/d063918c34f8d803d00821e6b0676f1501a25f97

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0e26ab46cbb7a6cff4fc129bf11c3d3cd86b6ab5&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=152345824

Comment 21

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

Comment on attachment 8935580 [details]
Bug 1390478 - never block the UI waiting for the FxA profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206460/diff/1-2/

Comment 22

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

Comment on attachment 8935580 [details]
Bug 1390478 - never block the UI waiting for the FxA profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206460/diff/2-3/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36187d67ca82
never block the UI waiting for the FxA profile. r=eoger

Comment 24

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/36187d67ca82