- Status Closed
- Percent Complete
- Task Type Bugs
- Category User Interface → Themes
- Assigned To No-one
- Operating System All players
- Severity Low
- Priority Very Low
- Reported Version Release 3.6
- Due in Version Undecided
-
Due Date
Undecided
- Votes
- Private
FS#11590 - Skin parser chokes on too many nested conditionals (Ondio at least)
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 .
2010-09-26 06:43
Reason for closing: Fixed
Additional comments about closing: Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407
in r28167
Loading...
Available keyboard shortcuts
- Alt + ⇧ Shift + l Login Dialog / Logout
- Alt + ⇧ Shift + a Add new task
- Alt + ⇧ Shift + m My searches
- Alt + ⇧ Shift + t focus taskid search
Tasklist
- o open selected task
- j move cursor down
- k move cursor up
Task Details
- n Next task
- p Previous task
- Alt + ⇧ Shift + e ↵ Enter Edit this task
- Alt + ⇧ Shift + w watch task
- Alt + ⇧ Shift + y Close Task
Task Editing
- Alt + ⇧ Shift + s save task
I’m very surprised that diff fixes it, but if it actually does then so should this one which doesnt break %if
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.
are you 100% certain Lear’s diff fixed the problem? I honestly dont see how it could.
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”.
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…
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.