Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11590 - Skin parser chokes on too many nested conditionals (Ondio at least)

Attached to Project: Rockbox
Opened by Marianne Arnold (pixelma) - Tuesday, 31 August 2010, 19:36 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Sunday, 26 September 2010, 08:43 GMT+2
Task Type Bugs
Category Themes
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Release 3.6
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

After an update to a current build (r27868)I got a problem with my theme on the Ondio which showed as "when it actually has to start playback and go to the WPS, then the player freezes with the 'loading' splash". This did not happen with other theme and so I did a bit of testing and found that the following line with nested conditionals was the culprit - the theme worked correctly before. Further testing went on with a test.wps that only contained this one line. It turned out that if I reduce the level of nesting by one, I get to the WPS but the player freezes after finishing buffering. If the nesting is reduced one more level, everything works correctly.
---

Here is the history of what I tried:

1. original
%s%?iG<%?iC<%iC|%iG>|%?id<%id|%?ia<. %?d(1)<%d(1)|Root>|.. %?d(2)<%d(2)|%?d(1)<%d(1)|Root>>>>>

2. replaced some unusual tags like %iG and %iC with more common ones but unfortunately I didn't keep it

3. simplified one level - fails on buffering when it has to take the else path (no title tag)
%?it<%it|%?ia<. %?d(1)<%d(1)|Root>|.. %?d(2)<%d(2)|%?d(1)<%d(1)|Root>>>>

4. one level less and this works
%?ia<. %?d(1)<%d(1)|Root>|.. %?d(2)<%d(2)|%?d(1)<%d(1)|Root>>>
---

Together with Lear in IRC I narrowed down when the issue started and he had some ideas what was going on (see logs at http://www.rockbox.org/irc/log-20100826#21:17:48 - 21:17 till 23:06). The issue started with revision 27846 (the %?if tag addition), I had to backport a fix for playback on hwcodec players which helped finding this. For testing purposes Lear provided a small "fix" about which he said that it'll break if I use %?if though but helps finding the root of the problem - here's the diff: http://pastie.org/1118815 .
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Sunday, 26 September 2010, 08:43 GMT+2
Reason for closing:  Fixed
Additional comments about closing:  in r28167
Comment by Jonathan Gordon (jdgordon) - Wednesday, 15 September 2010, 13:06 GMT+2
I'm very surprised that diff fixes it, but if it actually does then so should this one which doesnt break %if
   fix.diff (3.5 KiB) 
 b/apps/gui/skin_engine/skin_tokens.c |   61 ++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 33 deletions(-)

Comment by Marianne Arnold (pixelma) - Friday, 17 September 2010, 01:54 GMT+2
I just tried a build with this patch on my Ondio (based on r28095) and the issue remains with my theme which uses the original nested line. A simpler theme does not freeze the player. Thanks for looking into this though.
Comment by Jonathan Gordon (jdgordon) - Friday, 17 September 2010, 02:47 GMT+2
are you 100% certain Lear's diff fixed the problem? I honestly dont see how it could.
Comment by Marianne Arnold (pixelma) - Friday, 17 September 2010, 23:34 GMT+2
It fixed the problem with r27899. When applied to r28095 instead of the patch above, I get a crash with address in an undefined area, somwhere "in the wild".
Comment by Magnus Holmgren (learman) - Saturday, 18 September 2010, 12:02 GMT+2
The problem is the "char temp_buf[MAX_PATH];" in the if-handling code. It increases the stack usage of get_token_value noticeably (and my diff reduces it), so the crash is probably due to a stack overflow. Changing a "switch" to an "if" (if I read your patch correctly) doesn't change stack usage (everything is allocated on function entry).

skin_render_line has a similar problem. tempbuf is only used in one specific case too. That buffer is smaller, but skin_render_line is called recursively (not quite sure under what conditions though), making the problem worse.

If the temp buffers can't easily be removed or shared (by having one common static buffer), one way to fix it would be to move the code that actually use them to a separate non-inlined function. Then they are only allocated when actually needed. alloca might work too (it works in the Tremor codec...). Neither completely removes the possibility of a stack overflow though...
Comment by Jonathan Gordon (jdgordon) - Saturday, 18 September 2010, 12:09 GMT+2
Oh, I thought gcc allocated stack variables in the scope they are defined in. I'll move the if handling into another function then.

skin_render_line() is indeed a problem and I actually want to make that non-recursive (with a local stack+loop) but it isnt exactly trivial to do.

Loading...