Bug 38256 – [GTK] Random failure on 'testdownload' unit test

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Mario Sanchez Prada]

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.

[Mario Sanchez Prada]

Created attachment 54553 [details]
Init timer to NULL on instance init

This small patch should fix this issue

[Mario Sanchez Prada]

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.

[Alejandro G. Castro]

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

[Mario Sanchez Prada]

(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.

[Mario Sanchez Prada]

Created attachment 54556 [details]
 Init timer to NULL on instance init

[Mario Sanchez Prada]

(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!

[Mario Sanchez Prada]

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.

[Xan Lopez]

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.

[Mario Sanchez Prada]

(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.

[Mario Sanchez Prada]

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 :-$

[Mario Sanchez Prada]

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...

[Xan Lopez]

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.

[Xan Lopez]

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?

[Mario Sanchez Prada]

(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.

[Mario Sanchez Prada]

(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.

[Mario Sanchez Prada]

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.

[Xan Lopez]

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 @@
>[phone]  Mario Sanchez Prada  <[e-mail]>
>+
>+        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

[Mario Sanchez Prada]

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

[Xan Lopez]

(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.

[Mario Sanchez Prada]

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

[Xan Lopez]

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

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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