GNOME Bugzilla – Bug 764814
Crashes fetching lyrics with '%' in them
Last modified: 2016-10-22 12:25:30 UTC
2 bugs, first, metrolyrics should do some encoding... The title of the media is '99% Invisible' (grilo-test-ui-0.3:28993): Grilo-WARNING **: [lua-library] grl-lua-library.c:496: Can't fetch element 1 (URL: http://www.metrolyrics.com/99%-Invisible-lyrics-Roman-Mars.html): 'Invalid request URI or header: Bad Request' (grilo-test-ui-0.3:28993): Grilo-WARNING **: [lua-library] grl-lua-library.c:1280: This Lyrics do not match our parser! Please file a bug! And crash:
+ Trace 236169
Looks like it crashes whenever there's an error. Still, the "Bad request" should be avoided.
Created attachment 325631 [details] [review] metrolyrics: encode artist/title for correct url
Created attachment 325632 [details] [review] tests: metrolyrics with encoded url
(In reply to Bastien Nocera from comment #1) > Looks like it crashes whenever there's an error. I'll get back to this later on!
(In reply to Bastien Nocera from comment #1) > Looks like it crashes whenever there's an error. Still, the "Bad request" > should be avoided. Finally got some time to look into it, this is probably the same as bug 764291. For some reason GRL_WARNING is aborting (changing the warnings to debug avoids the crash). I'll looking into why this started to happen...
Pretty sure this isn't GRL_WARNING() causing the crash, we don't even see what it's supposed to print ("calling source callback function fail: %s"...)
(In reply to Bastien Nocera from comment #6) > Pretty sure this isn't GRL_WARNING() causing the crash, we don't even see > what it's supposed to print ("calling source callback function fail: %s"...) Sorry, In my tiny 'reproducer' I had g_test_init() set... oh well :) The problem that I can see happening here is that we are calling grl_lua_operations_pcall witha non null GError*. Let me know if the patch that I'll attach shortly fixes your crash as well.
Created attachment 326968 [details] [review] lua-factory: fix crash on error On grl_net_wc_request_async callback, we could be using the GError pointer more then once in the function, which leads to non null value. This patch clears the GError pointer and add guards to grl_lua_operations_pcall.
Review of attachment 325631 [details] [review]: Sure.
Review of attachment 325632 [details] [review]: ::: tests/lua-factory/sources/test_lua_metrolyrics.c @@ +94,3 @@ + lyrics = get_lyrics (source, audios[i].artist, audios[i].title); + g_assert_nonnull (lyrics); Why would it not fail for the example you added? It would fail indeed. You might want to move the assertion below the == NULL check.
Review of attachment 326968 [details] [review]: Sure.
Created attachment 326971 [details] [review] tests: metrolyrics with encoded url
Review of attachment 326971 [details] [review]: Looks good!
Review of attachment 326971 [details] [review]: Sorry, I did not test this properly after the crash fix. This patch breaks the tests as the fetch fails [0]. I'll be fixing this soon. [0] Grilo-WARNING **: [lua-library] grl-lua-library.c:496: Can't fetch element 1 (URL: http://www.metrolyrics.com/99%25-Invisible-lyrics-Roman-Mars.html): 'Invalid request URI or header: Bad Request'
Created attachment 326997 [details] [review] metrolyrics: char '%' is invalid in metrolyrics url We should not include '%' or its encoded version to metrolyrics requests.
Created attachment 326998 [details] [review] tests: metrolyrics with encoded url
Created attachment 326999 [details] [review] tests: metrolyrics with bad url
Review of attachment 326997 [details] [review]: Sure.
Review of attachment 326998 [details] [review]: A little more text would help: "Check that we don't create invalid requests for the metrolyrics server"
Review of attachment 326999 [details] [review]: Is that a 404? or something else? It's not clear.
(In reply to Bastien Nocera from comment #20) > Review of attachment 326999 [details] [review] [review]: > > Is that a 404? or something else? It's not clear. Yes, It is a valid url but we get 404 from server due missing lyrics. I'll update the patch and send it again. Thanks for the reviews.
Created attachment 327015 [details] [review] tests: metrolyrics with encoded url Check that we don't create invalid requests for the metrolyrics server
Created attachment 327016 [details] [review] tests: metrolyrics with nonexistent lyrics Testing metrolyrics under bad requests or nonexistent lyrics. The expected reply from the server is the 404 'Not Found' response.
Attachment 325631 [details] pushed as 54b43dd - metrolyrics: encode artist/title for correct url Attachment 326968 [details] pushed as c30ada5 - lua-factory: fix crash on error Attachment 326997 [details] pushed as e80c26e - metrolyrics: char '%' is invalid in metrolyrics url
Review of attachment 327015 [details] [review]: Yep.
Review of attachment 327016 [details] [review]: Good.
Attachment 327015 [details] pushed as 7069733 - tests: metrolyrics with encoded url Attachment 327016 [details] pushed as b5b1b4b - tests: metrolyrics with nonexistent lyrics
*** Bug 764291 has been marked as a duplicate of this bug. ***