Page MenuHomePhabricator

toggle_view_mobile gives MediaWiki internal error. on mw:Extension:GuidedTour
Closed, ResolvedPublic

Description

Clicking "Mobile view" in the footer, e.g. https://m.mediawiki.org/w/index.php?title=Extension:GuidedTour&mobileaction=toggle_view_mobile from https://www.mediawiki.org/wiki/Extension:GuidedTour gives

MediaWiki internal error.

Exception caught inside exception handler.

Set $wgShowExceptionDetails = true; at the bottom of LocalSettings.php to show detailed debugging information.

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterDon't parse URL to manipulate query parameters
mediawiki/extensions/MobileFrontend : masterRevert "Don't parse URL to manipulate query parameters"
mediawiki/extensions/MobileFrontend : masterDon't parse URL to manipulate query parameters

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 11 2016, 12:53 AM

Strange. @Quiddity, do you see this with other MediaWiki.org Extension: pages or other pages?

I just tried going from https://www.mediawiki.org/wiki/Extension:ParserFunctions to https://m.mediawiki.org/w/index.php?title=Extension:ParserFunctions&mobileaction=toggle_view_mobile and the redirect worked, and went from desktop enwiki Main page with the footer link and those looked okay, too.

phuedx added a subscriber: phuedx.EditedMar 14 2016, 7:25 PM

P2759 is an event (with backtrace!) associated with the exception.

Quiddity renamed this task from toggle_view_mobile gives MediaWiki internal error. to toggle_view_mobile gives MediaWiki internal error. on mw:Extension:GuidedTour.Mar 14 2016, 10:15 PM

Strange. @Quiddity, do you see this with other MediaWiki.org Extension: pages or other pages?

Nope, just that one page. Sorry I didn't specify that in the initial report! :)

The problem appears to be in the brace expansion routine in the parser, wherein a sub-request is made to a special page in order to fetch its content. If configured, MobileFrontend injects some additional content into special pages, the routine for which will execute for the sub-request dispatched by the parser. Part of that routine involves getting the URL of the current request, which fails as one isn't set by default.

This appears to be a bug in the parser rather than a bug in MobileFrontend.

phuedx added a comment.EditedMar 15 2016, 7:02 AM

Here's a test page I've made that reproduces the bug: https://www.mediawiki.org/wiki/User:Phuedx_(WMF)/T129600. It's one line long:

{{Special:PrefixIndex/Extension:GuidedTour/}}

@phuedx, do we need to loop in somebody from Parsing?

@dr0ptp4kt That'd be wise. We can submit a change for review to broaden the discussion too.

@ssastry who would you recommend to assist?

Jdlrobson moved this task from Backlog to Bugs on the MobileFrontend board.Mar 16 2016, 9:42 PM
phuedx added a comment.EditedMar 17 2016, 10:24 AM

@ssastry, @dr0ptp4kt: This bug can both be fixed in MobileFrontend and make MobileContext::doToggling simpler too. Change incoming…

Change 277969 had a related patch set uploaded (by Phuedx):
Don't parse URL to manipulate query parameters

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

As well as the RuboCop errors, 277969 needs more detail adding to the message.

Change 277969 merged by jenkins-bot:
Don't parse URL to manipulate query parameters

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

@Jdlrobson I've responded to your first comment. I'm not inclined to mark the tests as integration tests as the switching steps should actually replace one of those damned cookie setting steps! I'll submit a follow-up change.

The problem appears to be in the brace expansion routine in the parser, wherein a sub-request is made to a special page in order to fetch its content. If configured, MobileFrontend injects some additional content into special pages, the routine for which will execute for the sub-request dispatched by the parser. Part of that routine involves getting the URL of the current request, which fails as one isn't set by default.
This appears to be a bug in the parser rather than a bug in MobileFrontend.

Looks like you fixed the problem with your changes in MobileFrontend.

I think you are suggesting that the bug in the parser is that the parser doesn't set a request URL on the FauxRequest .. but, looks like in all the uses of FauxRequest in core, only php unit sets a url on this object. It seems like it would be good to set url on the FauxRequest at least in the parser since otherwise the parser is leaking abstraction about the FauxRequest into hooks on Special Pages (hooks would have to know that Special Page parse is using a FauxRequest that might not be setting the request url). Maybe @Anomie, @Legoktm or @tstarling can say more about this.

Looks like you fixed the problem with your changes in MobileFrontend.
I think you are suggesting that the bug in the parser is that the parser doesn't set a request URL on the FauxRequest .. but, looks like in all the uses of FauxRequest in core, only php unit sets a url on this object. It seems like it would be good to set url on the FauxRequest at least in the parser since otherwise the parser is leaking abstraction about the FauxRequest into hooks on Special Pages (hooks would have to know that Special Page parse is using a FauxRequest that might not be setting the request url). Maybe @Anomie, @Legoktm or @tstarling can say more about this.

I agree. The line in MobileFrontend that was highlighting the problem has been removed both because it highlighted the issue and because it was unnecessary. Indeed, the majority of 277969 is concerned with adding missing integration tests.

I'm happy to submit a patch to core to set the URL of the sub-request to fix the issue at its root.

Further to your point about leaky abstractions, I'd add that FauxRequest#getRequestURL throwing an exception which isn't thrown by WebRequest#getRequestURL is an LSP violation – and it's an undocumented exception too!

since otherwise the parser is leaking abstraction about the FauxRequest into hooks on Special Pages (hooks would have to know that Special Page parse is using a FauxRequest that might not be setting the request url)

Well, not necessarily. We could just document for those hooks that it's only valid to access the query parameters (->getVal() and so on) out of the request.

One question with setting a URL on the FauxRequest is whether that URL should be the actual URL of the request, or a faked-up URL for the special page. Another question is whether you also want to copy over the cookies, the Session, the headers, the IP, and so on (e.g. change to a DerivativeRequest).

since otherwise the parser is leaking abstraction about the FauxRequest into hooks on Special Pages (hooks would have to know that Special Page parse is using a FauxRequest that might not be setting the request url)

Well, not necessarily. We could just document for those hooks that it's only valid to access the query parameters (->getVal() and so on) out of the request.

That works as well.

Change 278715 had a related patch set uploaded (by Phuedx):
Revert "Don't parse URL to manipulate query parameters"

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

Change 278715 merged by jenkins-bot:
Revert "Don't parse URL to manipulate query parameters"

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

Given that the nightly builds were failing all weekend, I reverted 277969 with a view to merging 277969 in with it as well as better documenting the change.

Change 278729 had a related patch set uploaded (by Phuedx):
Don't parse URL to manipulate query parameters

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

since otherwise the parser is leaking abstraction about the FauxRequest into hooks on Special Pages (hooks would have to know that Special Page parse is using a FauxRequest that might not be setting the request url)

Well, not necessarily. We could just document for those hooks that it's only valid to access the query parameters (->getVal() and so on) out of the request.
One question with setting a URL on the FauxRequest is whether that URL should be the actual URL of the request, or a faked-up URL for the special page. Another question is whether you also want to copy over the cookies, the Session, the headers, the IP, and so on (e.g. change to a DerivativeRequest).

Preferably, a faked up URL for the special page. The alternative, dispatching a request to a special page despite the URL not being for the special page. doesn't make sense – neither does dispatching a request to a special page without a URL…

It's likely that I'm misunderstanding but, depending on the special page being transcluded, switching to DerivativeRequest might vary the parser output depending on if the user is logged in at parse time. If we were to switch to DerivativeRequest, then shouldn't it be for a logged out user?

Didn't +2 it yet as I have an open question.

Change 278729 merged by jenkins-bot:
Don't parse URL to manipulate query parameters

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

Will sign off Tuesday when this change rolls out.

@phuedx which URL(s) can I use to confirm?

@dr0ptp4kt http://reading-web-staging.wmflabs.org/wiki/T129600 is a copy of the page that I could reproduce the bug from my local environment.

Jdlrobson reopened this task as Open.EditedMar 29 2016, 5:27 PM

I want to make sure that mediawiki.org is no longer erroring and polluting logs before signing off.

Yes, yes, yes. Thanks guys.

Jdlrobson closed this task as Resolved.Mar 30 2016, 4:48 PM

It's now fixed on MediaWiki.org. Boom.