Page MenuHomePhabricator

Mathoid seems to crash for input $\textstyle{}$
Closed, ResolvedPublic

Description

How to reproduce

curl https://en.wikipedia.org/api/rest_v1/media/math/render/svg/b5a503b0fbbb58ad91eb00328b1ddb2ccdf5ef19

reported in T131177

Event Timeline

Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptMay 7 2016, 3:36 PM

The SVG is generated ok. An empty image. But the SVG to PNG conversion fails.

@AuFCL do you have a suggestion what should happen? I could generate a fallback png image. But I think it would be better to just error out.

AuFCL added a comment.May 7 2016, 10:38 PM

@Physikerwelt:
I am guessing here but there likely are only a few legacy usages of \…{}—and those probably result from poor template or editing choices and will be picked up and addressed soon enough on a needs basis.

I am not so keen on generation and regeneration of fallback PNG images "in case"—wouldn't this end up just churning cache pointlessly?

All in all I prefer the catch-failure-and-error-out approach.

@AuFCL I looked up how to use \textstyle for instance and it turns out that you would use it in the following way:
{\textstyle text to typeset} and not \textstyle{text to typeset} so it's not as easy to ensure it's correct use and I a can not use the same code that is used for the \text command that requires one argument.

Unfortunately, the whole program carshes on the attempt to convert the SVG to a PNG. I have tracked down the problem to the node-rsvg library.
https://github.com/physikerwelt/node-rsvg/commit/3b30bdc7525e5cfa144c7536dfd4a8b45d9810d5
https://travis-ci.org/physikerwelt/node-rsvg/jobs/128647935
but to fix that I'll need help.

AuFCL added a comment.EditedMay 8 2016, 9:55 PM

@Physikerwelt I do quite agree on the syntax detail. Unfortunately the prior behaviour (accepting optional closure in {} even though not demanded after a \displaystyle, \scriptstyle, or \textstyle directive) happened to be gracefully handled by the prior codebase, which sets a certain expectation that future developments not change this behaviour.

Neither of the following currently cause problems but constructs like <math>\sqrt{2}</math> are really shorthand for <math>\sqrt[]{2}</math>. I don't think users should need to be aware that certain empty structures are prone to implode, whilst others are "safe." Is this an unreasonable position to take?

(I had better make it explicit that I am not asking you to get to the bottom of the conversion SVG→PNG failure. I have already conceded earlier that simply returning an error is an acceptable compromise.)

@AuFCL I think we agree on what should be possible. The math extension always had problems to render empty math expressions. From what I currently see \textstyle is a valid input and should not cause problems.

I do not understand the connetion to the sqrt example. Try the following in a LaTeX editor

$\scriptstyle$

${\scriptstyle scriptstyle}: not\ scriptstyle $

$\scriptstyle{ scriptstyle}: scriptstyle$

cf https://www.authorea.com/users/94879/articles/111860/_show_article

Change 287952 had a related patch set uploaded (by Physikerwelt):
Catch exceptions thrown by restbase

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

AuFCL added a comment.May 10 2016, 5:55 PM

I do not understand the connetion to the sqrt example. Try the following in a LaTeX editor

Please do not worry about this. It was a (perhaps misguided) attempt to point out how including or excluding empty elements—here [] rather than {}—ideally should not affect the resultant rendering. Pardon adding to the confusion.

Change 287952 merged by jenkins-bot:
Catch exceptions thrown by restbase

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

mobrovac triaged this task as High priority.
mobrovac edited projects, added User-mobrovac; removed Patch-For-Review.

Change 289640 had a related patch set uploaded (by Mobrovac):
Handle MathJax errors and exceptions properly

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

Change 289648 had a related patch set uploaded (by Mobrovac):
Use the temporary MathJax-node branch with Rsvg fixes

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

I managed to get to the bottom of this. The root cause lies in librsvg's node bindings not stopping its rendering process on errors. MathJax-node compounds this by not properly handling errors itself, and the same for Mathoid. So, the complete fix includes:

Physikerwelt added a comment.EditedMay 19 2016, 1:56 PM

@mobrovac cool. How did figure that out in the librsvg code? I had no clue how to debug the c-code.
I have reviewed Gerrit 289640 and Gerrit 289648
Note, that Gerrit 289640 includes the functionality of PR #5 and PR #23
Moroever, I tested that on our test server:
Restbase shows now a reasonable error message, while the behavior from the Math extension is [unchanged]()http://beta.math.wmflabs.org:8080/wiki/User:Admin.

This comment was removed by Physikerwelt.

Change 289648 merged by jenkins-bot:
Use the temporary MathJax-node branch with Rsvg fixes

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

@mobrovac cool. How did figure that out in the librsvg code? I had no clue how to debug the c-code.

There are no limits to what one can do armed with a lot of patience and plenty of printf statements :)

Moroever, I tested that on our test server:
Restbase shows now a reasonable error message, while the behavior from the Math extension is [unchanged]()http://beta.math.wmflabs.org:8080/wiki/User:Admin.

That's great, but we will need to fix that in the extension. Specifically, res.body.detail in this case provides a really informative information on which the user might be able to act. Furthermore, Cannot get mml. Server problem. is simply wrong: it's not about MML nor is it a server problem.

mobrovac closed this task as Resolved.May 19 2016, 3:12 PM
mobrovac removed a project: Patch-For-Review.
mobrovac removed a subscriber: gerritbot.
AuFCL removed a subscriber: AuFCL.Nov 21 2016, 7:40 PM