Triggered by 530109?

cc @Simetrical @daniel @Krinkle

That's pretty weird, as a first glympse, I don't see where MobileFrontend.AMC.Manager depends on MobileFrontend.FeaturesManager

The question is, did this work before, or crash with a stack overflow? Do we log all fatal errors in such a way that we know these page views actually succeeded before this change, or might the error just have changed?

I did consider when writing the check that it's possible for recursion to be happening in a way that terminates, in which case the check would break otherwise working pages, but it seemed unlikely.

In T231014#5431736, @pmiazga wrote:

That's pretty weird, as a first glympse, I don't see where MobileFrontend.AMC.Manager depends on MobileFrontend.FeaturesManager

Did you look at the stack trace?

#6 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(427): Wikimedia\Services\ServiceContainer->createService(string)
#7 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/specials/SpecialMobileHistory.php(94): Wikimedia\Services\ServiceContainer->getService(string)
#8 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/MobileContext.php(364): SpecialMobileHistory::shouldUseSpecialHistory(Title, User)
#9 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/MobileContext.php(339): MobileContext->redirectMobileEnabledPages()
#10 /srv/mediawiki/php-1.34.0-wmf.19/extensions/MobileFrontend/includes/ServiceWiring.php(48): MobileContext->shouldDisplayMobileView()
#11 /srv/mediawiki/php-1.34.0-wmf.19/includes/libs/services/ServiceContainer.php(459): Closure$#4(MediaWiki\MediaWikiServices)

MobileFrontend.AMC.Manager's instantiation function calls MobileFrontend.Context's shouldDisplayMobileView() method. That method, if $this->shouldDisplayMobileViewInternal() returns true, calls $this->redirectMobileEnabledPages(). If the request is for action=history and the title is not null, this then calls SpecialMobileHistory::shouldUseSpecialHistory(). That then tries to instantiate MobileFrontend.FeaturesManager.

So this looks like it will occur on any attempt to view a history page on mobile. But it also looks like it would previously have worked despite the circular dependency. When it tried to instantiate MobileFrontend.AMC.Manager the second time, shouldDisplayMobileView() would short-circuit because $this->mobileView is already set. This would allow all the necessary services to be created just fine, although I don't know if it would result in any bugs.

In short, it looks like the code would previously have worked, but the current behavior is probably not intended and it's good that we caught it. I suggest:

1. We add a bit of tolerance to the recursion check, and allow it to continue to some reasonable stack depth before terminating it even if we find a circular dependency. A circular dependency should log a warning in any event. I'll work on that now.

2. If desired, the commit a995d9be1df can be reverted for now. There should be no conflicts or likelihood of bad stuff happening, the change is simple and self-contained and doesn't affect normal operation.

3. The developers of MobileFrontend might want to consider if their code is correct here. (If SpecialMobileHistory was an instance method of something that got its services injected instead of a static method that used MediaWikiServices, the circularity would have been immediately obvious.)

Change 531729 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Warn on circular service dependencies, don't throw

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

@Simetrical thx for the comment, yeah, you're right, I missed that SpecialMobileHistory::shouldUseSpecialHistory gets the FeatureManager. I only focused on ServiceWiring and AMC codebase. I'll fix that code. circular dependencies are bad and such situation should not have place.

Change 531729 abandoned by simetrical:
Warn on circular service dependencies, don't throw

Reason:
If we want it to be fatal, I don't have a problem with that.

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

Change 532389 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Postpone call to MobileContext::shouldDisplayMobileView()

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

Friendly reminder, this is blocking the train. Will it be resolved by train window tomorrow?

@zeljkofilipin yes, the fix got merged like 2 hours ago, but the CI is still processing it ;/

Change 532389 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Postpone call to MobileContext::shouldDisplayMobileView()

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

@zeljkofilipin this should be fixed now, can we resolve task once this goes to production? We're unable to reproduce it locally.

@pmiazga if you think it's fixed, resolve the task. I'll reopen it if I see the error in the logs after the deployment.

ok, thx for info

Seeing this again in production on enwiki as of about 15:15 UTC today https://logstash.wikimedia.org/goto/971e0dc9c8f3d9cfe4d76c30a6446a9b

5xx dashboard: https://logstash.wikimedia.org/goto/d77ad9c51613ef72b49395fc4ed5218e

Change 533064 had a related patch set uploaded (by Jforrester; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@wmf/1.34.0-wmf.19] Postpone call to MobileContext::shouldDisplayMobileView()

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

Change 533064 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@wmf/1.34.0-wmf.19] Postpone call to MobileContext::shouldDisplayMobileView()

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

Mentioned in SAL (#wikimedia-operations) [2019-08-28T17:06:14Z] <jforrester@deploy1001> Synchronized php-1.34.0-wmf.19/extensions/MobileFrontend/includes: T231014 Postpone call to MobileContext::shouldDisplayMobileView() (duration: 00m 55s)

Error rate back to nominal.