1465355 - Treeherder asks to log in every time a tree is accessed

Status: RESOLVED FIXED

(Tree Management :: Treeherder: Frontend, defect, P1)

Description

[Natalia Csoregi [:nataliaCs]]

We noticed today that treeherder might have a logging issue, and we tested several scenarios.
 For example:
1. accessed m-c, logged in, accessed m-i from the same tab, and had to log in again
2. accessed autoland, logged in, left that one open and opened a new tab for m-i, had to log in again
3. accessed m-i, logged in, closed tab, opened m-c, had to log in again.

The console doesn't seem to provide any clue why it's failing.

Comment 1

[Ed Morley [:emorley]]

Hi!

Testing now shows login works fine for me - so some more information would be useful:
* does this occur just for you, or for others too (the "we" above could be read either way :-))
* when did this start and does it occur 100% of the time, or just some of the time?
* what browser and version are you using? (when reporting an issue with a website, this is a must-have piece of information to include)
* are there custom cookie/localstorage/referrer/... settings being used?
* does any of the following help?
  - disabling addons/using the browser's safe mode
  - using a different browser

Comment 2

[Bogdan Tara[:bogdan_tara | bogdant]]

This happens to Aryx, me, :nataliaCs and :RaulGurzau at this time.
Just tested on ff 60.0.1 (64-bit) and Opera 52.0.2871.64.
Steps to repro:
 - Login in th;
 -  open a new th tab (eg: copy url and paste it in a new tab).
Observe that the user is not logged in the new th tab.

This also reproduces on Ubuntu, FF 60.0.1 (64-bit) - no addons here.

Also, refreshing the logged in tab seems to log out the user.

Comment 3

[Ed Morley [:emorley]]

I can't reproduce, but I've found this change in the auth0 update that was included in today's prod push that looks like a possible cause:
https://github.com/auth0/auth0.js/pull/758

I'll deploy a revert of that to stage now (will take 5 mins to be live) to see if it fixes the problem. Whilst we're waiting for the stage deploy it would be useful if you could try and reproduce on stage (just to provide a control).

Comment 4

[Bogdan Tara[:bogdan_tara | bogdant]]

I also saw strange stuff from auth0 (white page showing ok).

What's the link for stage?

Comment 5

[Ed Morley [:emorley]]

Ah sorry forgot to paste it:
https://treeherder.allizom.org/

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

Thank you.

Also reproduces there too.

Comment 7

[Ed Morley [:emorley]]

Great :-)

The auth0js version revert has now just been deployed to stage - could you try and again and see if it's hopefully fixed?

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

Just tested. It seems it's not fixed.
Trying to open another tree via the repos menu logs out the user.

Comment 9

[Ed Morley [:emorley]]

Could you clear the cache and try again?

Also, just checking this only started today?

Comment 10

[Ed Morley [:emorley]]

Also, the question about percentage occurrence wasn't answered either?

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

Just tried again on ff and opera. Still not ok.

This seems to have started today. It seemed to work ok about 5 hours ago.

For us (sheriffs) it occurs 100%. 

Additionally, th does not save our failure classifications.

Comment 12

[Ed Morley [:emorley]]

One other possible cause is that part of bug 1450039 replaced ThUserModel with UserModel:
https://github.com/mozilla/treeherder/blob/1badec2b45977144dcbaa15d4eefb47f3da8e934/ui/js/models/user.js
https://github.com/mozilla/treeherder/blob/eddbc1c8a773d0d9ab553a32bf9b943b5416785a/ui/models/user.js

...and that model is used as part of determining whether a user is logged in on the backend (and as such, whether to reset the login state on the frontend):
https://github.com/mozilla/treeherder/blob/eddbc1c8a773d0d9ab553a32bf9b943b5416785a/ui/js/components/auth.js#L79-L86

Whilst the new implementation looks equivalent, perhaps there are subtle changes that are causing this?

If someone who is able to reproduce could bisect locally using `yarn start`, it would be really useful.

Comment 13

[Ed Morley [:emorley]]

I've now deployed a revert of bug 1450039 on stage instead - could you try that? :-)

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

That fixes the permafail with Firefox 60.0.1

Comment 15

[Natalia Csoregi [:nataliaCs]]

Looks good on stage

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

Looks good on stage.

Comment 17

[Ed Morley [:emorley]]

Great! I have a prod deploy in progress of that revert - once it's finished the bot will comment in #treeherder and an email will be sent to the tools-treeherder mailing list (it may be a minute or two after the notification before the code is fully live, due to our use of release phase).

Comment 18

[Ed Morley [:emorley]]

The deploy of the revert to prod has now completed. Hopefully this issue should be resolved - sorry for the hassle!

Cameron - it looks like bug 1450039 can cause login to get reset for some users. I couldn't reproduce locally using Firefox Nightly, but perhaps it's a browser version issue or Angular digest related race condition?

Currently bug 1450039 is just reverted on the production branch (to reduce churn on master), so we'll need to make sure we don't do any other deploys straight from master until either a fix lands there or we decide to revert for real.

Comment 19

[Cameron Dawson [:camd]]

I think there must be a race condition here.  I hadn't seen this, but I'm able to reproduce now.  Working on this now.

Sorry for the hassle, everyone.  And thanks for doing the prod revert, Ed.

Comment 20

[Cameron Dawson [:camd]]

Hmm, yeah this seems related to the version of FF, afaict.  FF 60.x has the problem.  But nightly at 62.x works fine.  So at least there's a reproducible test case there.

Comment 21

[Cameron Dawson [:camd]]

It appears to be this change new to FF 62 (currently beta):

https://developer.mozilla.org/en-US/Firefox/Releases/61

Which contains the item:
* The Fetch API's Request.credentials property now defaults to "same-origin" per the latest revision of the specification (bug 1394399).

So if I send the { credentials: "same-origin" } with the call for the current user, that works in FF 61.  Still doing more testing, but this seems to have been the issue.

Comment 22

[Ed Morley [:emorley]]

Ah was just about to post this... haha.

---

Ah that explains why I couldn't reproduce.

I've used mozregression to find where this started working in Firefox, and got it down to:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d642b603b348dd8a8d69bf983b1b66ecfc9b7cc1&tochange=2feb276e4fcceb7badbc4b634129111656c60f62

In that range is bug 1394399 - which would explain the symptoms here.

So this should be able to be fixed by explicitly setting the credentials to `same-origin` rather than relying on the default.

It's unfortunate that the MDN page doesn't say that the default used to be different (otherwise we'd have perhaps spotted this):
https://developer.mozilla.org/en-US/docs/Web/API/Request/credentials

Comment 23

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3593

Comment 24

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/7eb466affd119031952a18a3a9426c5d6ed112ba
Bug 1465355 - Fix Treeherder forgetting being logged in (#3593)