It seems that the buildbot is randomly finding problems with the WebKitGTK unit tests related to the WebKitDownload component, mainly because the internal timer in the WebKitDownload object might be, under some obscure circunstances, not properly set to NULL when calling to webkit_download_start(). We need to find a way to make sure no weird things can happen so the unit tests don't randomly fail anymore, at least in this way.
Created attachment 54553 [details] Init timer to NULL on instance init This small patch should fix this issue
Created attachment 54554 [details] Make sure the set_filename function is called after handling the 'download-requested' signal for the asynchronous test Make sure the set_filename function is called after handling the 'download-requested' signal for the asynchronous test.
FYI, you can find the stack of one of the crashes here: http://webkit-bots.igalia.com/amd64/svn_58244.core-when_1272281034-_-who_testdownload-_-why_6.12932.trace.html
(In reply to comment #2) > Created an attachment (id=54554) [details] > Make sure the set_filename function is called after handling the > 'download-requested' signal for the asynchronous test > [...] Sorry for the noise in this duplicated description, and because of forgetting to explain the reason of this one: This second patch is related to the same issue because it could happen, with the current code, that the set_filename () function got called too early in the mainloop, therefore screwing the tests because the 'theDownload' global variable wouldn't be properly set yet. With this modification we ensure the set_filename() function gets called always after handling the download-requested signal, therefore avoiding potential problems. Perhaps this patch should go along with a different bug, but I'm not completely sure about it. if so, please let me know and I'll gladly file a new one.
Created attachment 54556 [details] Init timer to NULL on instance init
(In reply to comment #5) > Created an attachment (id=54556) [details] > Init timer to NULL on instance init Uploaded a new version using 0 instead of NULL. Sorry for the spam!
Created attachment 54557 [details] Init timer to 0 on instance init Ladies and gentlemen! when it seemed it would be impossible to screw it up three times in a row, I did it! :-) New version of the patch with both the ChangeLog and git log message updated accordingly.
Comment on attachment 54557 [details] Init timer to 0 on instance init AFAIK private instance data is zeroed when created, so if this fixes the crasher there's something really weird going on.
(In reply to comment #8) > (From update of attachment 54557 [details]) > AFAIK private instance data is zeroed when created, so if this fixes the > crasher there's something really weird going on. Yes, but anyway explicitly setting that to 0 won't hurt either :-) Anyway, there's something wrong in that test anyway, since something bad could happen if set_filename() got executed prior to the download-requested signal handler, which could also be behind this random behaviour.. If that was the case, the second patch should help.
Created attachment 55274 [details] 1. Init timer to 0 on instance init Arrgh!! Today I realized the number of the bug was wrong in the Changelog. New patch then :-$
Created attachment 55275 [details] 2. Make sure the set_filename function is called after handling the 'download-requested' signal for the asynchronous test Second patch...
Comment on attachment 55274 [details] 1. Init timer to 0 on instance init We cannot got this in like this. The ChangeLog message is not useful because you don't explain why are you doing that; you could say it's just for consistency-sake at the very least, although that would be false since we don't really consistently init all private members to zero. Since this does not really fix anything I think we should not get it in unless we stablish some policy about doing this everywhere.
Comment on attachment 55275 [details] 2. Make sure the set_filename function is called after handling the 'download-requested' signal for the asynchronous test Seems the ChangeLog is hosed in the patch. Also, can you explain there why are you doing what you do?
(In reply to comment #12) > (From update of attachment 55274 [details]) > We cannot got this in like this. The ChangeLog message is not useful because you don't explain why are you doing that; you could say it's just for consistency-sake at the very least, although that would be false since we don't really consistently init all private members to zero. > > Since this does not really fix anything I think we should not get it in unless we stablish some policy about doing this everywhere. Fair enough. Your rationale sounds good to me and, on top of that, I don't have any strong reason to ge this in apart than the "feeling" than doing it so would be a good idea. But I'm new to this and of course perhaps it makes no sense at all and, as you say, it wouldn't fix anything anyway, so let's keep things coherent and avoid this one.
(In reply to comment #13) > (From update of attachment 55275 [details]) > Seems the ChangeLog is hosed in the patch. What do you mean? > Also, can you explain there why are you doing what you do? Well, as I said in comment #4: "This second patch is related to the same issue because it could happen, with the current code, that the set_filename () function got called too early in the mainloop, therefore screwing the tests because the 'theDownload' global variable wouldn't be properly set yet. With this modification we ensure the set_filename() function gets called always after handling the download-requested signal, therefore avoiding potential problems." So, the point is about making sure things happen in the right moment, instead of trusting the main loop to execute the idle function in the right time.
Created attachment 55972 [details] Make sure the set_filename function is called after handling the 'download-requested' signal for the asynchronous test (In reply to comment #15) > (In reply to comment #13) > > (From update of attachment 55275 [details] [details]) > > Seems the ChangeLog is hosed in the patch. > > What do you mean? > > > Also, can you explain there why are you doing what you do? After talking to Xan via IRC I now understand his question was about the problems with the ChangeLog and not the patch itself, so now I'm attaching a new version of the second patch, hopefully a valid one, and non dependant on the first patch, as the other one.
Comment on attachment 55972 [details] Make sure the set_filename function is called after handling the 'download-requested' signal for the asynchronous test >--- a/WebKit/gtk/ChangeLog >+++ b/WebKit/gtk/ChangeLog >@@ -1,3 +1,21 @@ >+2010-05-06 Mario Sanchez Prada <msanchez@igalia.com> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ [GTK] Random failure on 'testdownload' unit test >+ https://bugs.webkit.org/show_bug.cgi?id=38256 >+ >+ Make sure the set_filename function is called after >+ handling the 'download-requested' signal for the >+ asynchronous test. >+ >+ * tests/testdownload.c: >+ (set_filename): >+ (handle_download_requested_cb): >+ (download_requested_cb): >+ (download_requested_asynch_cb): >+ (test_webkit_download_perform): >+ As commented, it would be nice to add here the explanation in comment #15, since you already wrote it. >+ g_idle_add((GSourceFunc)set_filename, g_strdup(temporaryFilename)); Mmm, do you really need to dup the string? >+ } else { >+ gchar *uri = g_filename_to_uri(temporaryFilename, NULL, NULL); >+ if (uri) >+ webkit_download_set_destination_uri(download, uri); >+ g_free(uri); >+ } > } > >+ GCallback dr_cb = NULL; downloadRequestCallback? or downloadRequestCB? or something not 'dr_cb' :D
Created attachment 60009 [details] Make sure the set_filename function is called after handling the 'download-requested' signal for the asynchronous test (In reply to comment #17) > [...] > As commented, it would be nice to add here the explanation in comment #15, since you already wrote it. Done, sorry. > >+ g_idle_add((GSourceFunc)set_filename, g_strdup(temporaryFilename)); > > Mmm, do you really need to dup the string? As the string is gonna be used in a function called "on idle", I'd rather do it this way and then free the string there. I think it's safer. > [...] > >+ GCallback dr_cb = NULL; > > downloadRequestCallback? or downloadRequestCB? or something not 'dr_cb' :D Sure! Done. Attached new patch addressing these issues
(In reply to comment #18) > > > > Mmm, do you really need to dup the string? > > As the string is gonna be used in a function called "on idle", I'd rather do it this way and then free the string there. I think it's safer. AFAICT the string survives across the whole test, so whether it's used in an idle or not is irrelevant here.
Created attachment 60024 [details] Make sure the set_filename function is called after handling the 'download-requested' signal for the asynchronous test (In reply to comment #19) > (In reply to comment #18) > > > > > > Mmm, do you really need to dup the string? > > > > As the string is gonna be used in a function called "on idle", I'd rather do it this way and then free the string there. I think it's safer. > > AFAICT the string survives across the whole test, so whether it's used in an idle or not is irrelevant here. Ok, you convinced me :-) Here you have the new patch then
Comment on attachment 60024 [details] Make sure the set_filename function is called after handling the 'download-requested' signal for the asynchronous test r=me
Comment on attachment 60024 [details] Make sure the set_filename function is called after handling the 'download-requested' signal for the asynchronous test Clearing flags on attachment: 60024 Committed r62167: <http://trac.webkit.org/changeset/62167>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/62167 might have broken Qt Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/62168 http://trac.webkit.org/changeset/62166 http://trac.webkit.org/changeset/62167