Change 289904 had a related patch set uploaded (by Gergő Tisza):
Disable MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/289904

Change 289904 merged by jenkins-bot:
Disable MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/289904

I'm baffled @Tgr @Florian. Didn't this patch go out with a new release of MediaWiki ? So does that mean we shipped something with a broken MobileFrontend test? I would hope a broken test would block a release, so if this isn't the case why hasn't it done so?

The decision to disable the test blindly without understanding the test and what it protected is in my opinion a lazy and ill-made decision.

cc @BBlack the test "in desktop view the cookie vary header should never be set" is concerning as this could cause us all sorts of caching issues if true. Hopefully it's a false positive but we may want to stop the train until we can validate that.

@Jdlrobson thanks for the heads-up, and yes, that's a very serious issue. Vary must unconditionally be sent with the same value for any given URI. Varying a Vary header is a recipe for disaster.

(Also, why does MFE mess with Vary:Cookie? that should be a core thing, right?)

In T135866#2320598, @Jdlrobson wrote:

I'm baffled @Tgr @Florian. Didn't this patch go out with a new release of MediaWiki ? So does that mean we shipped something with a broken MobileFrontend test? I would hope a broken test would block a release, so if this isn't the case why hasn't it done so?

MobileFrontend is not bundled with the MW tarballs, so we didn't ship a broken anything.

@demon but we cut extension branches at the same time no? So if someone upgrades to mediawiki with MobileFrontend they could potentially have this issue?

@BBlack am going to look into this in the afternoon to see if something has indeed been broken. It messes with the vary cookie for sites which do not have a mobile site url but use MobileFrontend. It should only apply for 3rd parties so am a little worried that may have gone wrong. Will update as soon as I know more!

In T135866#2320715, @Jdlrobson wrote:

@demon but we cut extension branches at the same time no? So if someone upgrades to mediawiki with MobileFrontend they could potentially have this issue?

This was a security release so no new branches were cut.

okay thanks for clarifying @demon !

In T135866#2320677, @BBlack wrote:

@Jdlrobson thanks for the heads-up, and yes, that's a very serious issue. Vary must unconditionally be sent with the same value for any given URI. Varying a Vary header is a recipe for disaster.

It seems https://github.com/wikimedia/mediawiki/commit/af3b10287e4b4de8360ea57c400c7b8ed71596a1 adds the Cookie Vary Header unconditionally if getCacheVaryCookies returns something. Thus, thankfully this does appear to be a false positive and it is my opinion that it can be safely removed.

(The above assumes that the fix to T125283 is sane)

Change 290285 had a related patch set uploaded (by Jdlrobson):
Revert "Disable MobileFrontendHooksTest::testOnBeforePageDisplay"

https://gerrit.wikimedia.org/r/290285

As I said in the commit summary,

The test tries to assert that no Vary headers are set in OutputPage,
which is not something a test should be doing without lots of mocking
(OutputPage is not isolated very well and MobileFrontend is certainly
not the only component messing with headers).

It was clear the test was wrong; it wasn't clear what would be right (does MF add a vary header under certain circumstances and was the test asserting that that does not happen under certain other circumstances?); it was breaking CI and blocking the work of all WMF developers, so something had to be done about it. I'm not sure what else could have been done differently. The point of UBN bugs is that they should be handled fast (and ideally by the team who owns the component - I tried to call attention to it on #wikimedia-mobile, but might have been too subtle about it). Even if I wasn't sure the test is a false positive (and I was - maybe the commit summary did not express that clearly), whether it is significant enough to make the issue a release blocker is not a call that I can make, with zero knowledge of the product. That should be done by someone familiar with MF.

The timing of your reply suggests that the web team only became aware of this issue at the weekly triage meeting. If you are looking for improvements to the process, IMO that's the place to start. There should be at least one person in Reading Web (but preferably more, to account for vacations) who gets notifications about UBN bugs. You can set up a Herald rule to do that.

In T135866#2321258, @Tgr wrote:

As I said in the commit summary,

The test tries to assert that no Vary headers are set in OutputPage,
which is not something a test should be doing without lots of mocking
(OutputPage is not isolated very well and MobileFrontend is certainly
not the only component messing with headers).

It was clear the test was wrong; it wasn't clear what would be right (does MF add a vary header under certain circumstances and was the test asserting that that does not happen under certain other circumstances?); it was breaking CI and blocking the work of all WMF developers, so something had to be done about it. I'm not sure what else could have been done differently. The point of UBN bugs is that they should be handled fast (and ideally by the team who owns the component - I tried to call attention to it on #wikimedia-mobile, but might have been too subtle about it). Even if I wasn't sure the test is a false positive (and I was - maybe the commit summary did not express that clearly), whether it is significant enough to make the issue a release blocker is not a call that I can make, with zero knowledge of the product. That should be done by someone familiar with MF.

The timing of your reply suggests that the web team only became aware of this issue at the weekly triage meeting. If you are looking for improvements to the process, IMO that's the place to start. There should be at least one person in Reading Web (but preferably more, to account for vacations) who gets notifications about UBN bugs. You can set up a Herald rule to do that.

We have a triage every morning except Friday which is arguably a bad day to be pushing potentially breaking changes. I was personally at an event Friday. I'd suggest contacting Adam in future for anything urgent like this.

Anyway, yes it seems like there is nothing to worry about in this particular instance. The original revert hadn't given me the confidence I needed to but it seems you handled this affair correctly so thank you for taking care of it. Hopefully rolling this out won't prove otherwise due to some badly undocumented piece of MobileFrontend code :)

Change 290496 had a related patch set uploaded (by Gergő Tisza):
Remove Vary header check from MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/290496

Change 290285 merged by jenkins-bot:
Revert "Disable MobileFrontendHooksTest::testOnBeforePageDisplay"

https://gerrit.wikimedia.org/r/290285

Change 290496 merged by jenkins-bot:
Remove Vary header check from MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/290496

Change 290579 had a related patch set uploaded (by Gergő Tisza):
Remove Vary header check from MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/290579

Change 290579 merged by Brian Wolff:
Remove Vary header check from MobileFrontendHooksTest::testOnBeforePageDisplay

https://gerrit.wikimedia.org/r/290579