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 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.
(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.
(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
2010-04-28 05:53 PDT, Mario Sanchez Prada
2010-04-28 05:55 PDT, Mario Sanchez Prada
2010-04-28 06:04 PDT, Mario Sanchez Prada
2010-04-28 06:14 PDT, Mario Sanchez Prada
2010-05-06 11:31 PDT, Mario Sanchez Prada
2010-05-06 11:32 PDT, Mario Sanchez Prada
2010-05-13 07:20 PDT, Mario Sanchez Prada
2010-06-29 04:59 PDT, Mario Sanchez Prada
2010-06-29 08:19 PDT, Mario Sanchez Prada