Hi, webkit trunk GtkLauncher crashes at startup in: #0 0xb70f4237 in WebCore::requestStartedCallback(_SoupSession*, _SoupMessage*, _SoupSocket*, void*) () from /home/arno/webkit/WebKit.upstream/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0 #1 0xb624c49a in _soup_marshal_VOID__OBJECT_OBJECT () from /usr/lib/i386-linux-gnu/libsoup-2.4.so.1 #2 0xb6085484 in g_closure_invoke () from /usr/lib/i386-linux-gnu/libgobject-2.0.so.0 There are two reasons for the crash: - soup_add_timeout is called with d->m_firstRequest.timeoutInterval() * 1000 which evaluates to 0 in my computer (instead of evaluating to INT_MAX) - WebKit crashes when a request timeout before request-started signal is emitted.
Created attachment 166517 [details] c testcase: outputs 0 when compiled with -msse2 -mfpmath=sse c testcase to understand why soup_add_timeout is called with a 0 argument. This c testcase outputs 0d on i386 platform when compiled with gcc -msse2 -mfpmath=sse overflow.c -o overflow when compiled with just gcc overflow.c -o overflow, it outputs 4294966296d As default timeout value is double INT_MAX, this explains why soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, handle); results in a 0 argument.
Created attachment 166519 [details] Patch patch proposal
CCing Sergio who also works on the libsoup backend.
Created attachment 166541 [details] Patch updated patch: fixes a warning when calling setDefersLoading(true) and there is no timeout source
*** Bug 98157 has been marked as a duplicate of this bug. ***
Comment on attachment 166541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166541&action=review LGTM, thanks for the fix - I just spotted the early handle deletion issue today. Work on bug 74802 is progressing. > Source/WebCore/ChangeLog:14 > + occur. So, reset soupMessage handle data in occurs > Source/WebCore/ChangeLog:22 > + cleanupSoupRequestOperation will be called automatically. Thanks for fixing this - I just noticed this in the duplicate of this bug. I plan to upload the test and patch for 74802 tomorrow. I'd change "and crash occur" -> and a crash occurs in sendRequestCallback due to an early deleted handle.
Comment on attachment 166541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166541&action=review Forgot to submit this part. (Note, I am not a reviewer, but maybe Martin or Gustavo can do the final review). > Source/WebCore/ChangeLog:18 > + deleted, and crash occur in sendRequestCallback. To avoid that, call -> and a crash occurs in sendRequestCallback due to an early destroyed handle.
Created attachment 166910 [details] Patch updated patch
Comment on attachment 166910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166910&action=review Okay. Seems reasonable, but consider the following. Please also follow the suggestion above. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:379 > + g_object_set_data(G_OBJECT(d->m_soupMessage.get()), "handle", NULL); Use 0 instead of NULL here. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:769 > - if (d->m_firstRequest.timeoutInterval() > 0) { > + guint timeoutInterval = d->m_firstRequest.timeoutInterval() * 1000; > + if (timeoutInterval > 0) { See below. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:878 > + guint timeoutInterval = d->m_firstRequest.timeoutInterval() * 1000; > + if (timeoutInterval > 0) { Maybe it would be clearer to simply check for INT_MAX explicitly and just leave a comment like: // INT_MAX * 1000 is zero on i386, so we explicitly check this situation. unsigned timeoutInterval = d->m_firstRequest.timeoutInterval(); if (timeoutInterval && timeoutInterval != INT_MAX) { > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1045 > + guint timeoutInterval = d->m_firstRequest.timeoutInterval() * 1000; > + if (timeoutInterval > 0) { Ditto.
(In reply to comment #9) > Maybe it would be clearer to simply check for INT_MAX explicitly and just leave a comment like: > > // INT_MAX * 1000 is zero on i386, so we explicitly check this situation. > unsigned timeoutInterval = d->m_firstRequest.timeoutInterval(); > if (timeoutInterval && timeoutInterval != INT_MAX) { > > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1045 > > + guint timeoutInterval = d->m_firstRequest.timeoutInterval() * 1000; > > + if (timeoutInterval > 0) { reading timeoutInterval as unsigned will yield wrong results when for example, timeoutInterval is defined as 0.5 also, I just realize if timeoutInterval is INT_MAX / 2 or ever INT_MAX / 100, it won't work either. So I'm thinking about something like: double timeoutInterval = d->m_firstRequest.timeoutInterval(); // overflowed integer may evaluate either to 0 either to INT_MAX, so we explicitly check this situation. if (timeoutInterval * 1000 > INT_MAX) { timeoutInterval = 0; }
Created attachment 166924 [details] Patch updated patch
Maybe a better fix here is just to change this: #if !PLATFORM(MAC) || USE(CFNETWORK) double ResourceRequestBase::s_defaultTimeoutInterval = INT_MAX; #else // Will use NSURLRequest default timeout unless set to a non-zero value with setDefaultTimeoutInterval(). double ResourceRequestBase::s_defaultTimeoutInterval = 0; #endif in ResourceRequestBase.cpp, so that the s_defaultTimeoutInterval for SOUP is 0.
Created attachment 166938 [details] Patch updated patch
Comment on attachment 166938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166938&action=review Great! This is much, much simpler. Just one small change, I think. As an aside, I think this ambiguity between platforms isn't so great. It'd be nice if there was a constant NoTimeout that consumers of this API could check against. That could be another patch though. > Source/WebCore/platform/network/ResourceRequestBase.cpp:44 > +#if USE(SOUP) > +// soup timeout is defined with integers milliseconds. We set 0 as default value to avoid integer overflow. > +double ResourceRequestBase::s_defaultTimeoutInterval = 0; > +#elif !PLATFORM(MAC) || USE(CFNETWORK) > double ResourceRequestBase::s_defaultTimeoutInterval = INT_MAX; > #else > // Will use NSURLRequest default timeout unless set to a non-zero value with setDefaultTimeoutInterval(). This could be folded to be: #if !USE(SOUP) && (!PLATFORM(MAC) || USE(CFNETWORK)) double ResourceRequestBase::s_defaultTimeoutInterval = INT_MAX; #else double ResourceRequestBase::s_defaultTimeoutInterval = INT_MAX; #endif
Created attachment 166943 [details] Patch updated patch
Comment on attachment 166943 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166943&action=review > Source/WebCore/platform/network/ResourceRequestBase.cpp:41 > +#if USE(SOUP) > +// soup timeout is defined with integers milliseconds. We set 0 as default value to avoid integer overflow. > +double ResourceRequestBase::s_defaultTimeoutInterval = 0; > +#elif !PLATFORM(MAC) || USE(CFNETWORK) :) This isn't quite what I asked for, but I'll make the change and land it for you.
(In reply to comment #16) > (From update of attachment 166943 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166943&action=review > > > Source/WebCore/platform/network/ResourceRequestBase.cpp:41 > > +#if USE(SOUP) > > +// soup timeout is defined with integers milliseconds. We set 0 as default value to avoid integer overflow. > > +double ResourceRequestBase::s_defaultTimeoutInterval = 0; > > +#elif !PLATFORM(MAC) || USE(CFNETWORK) > > :) This isn't quite what I asked for, but I'll make the change and land it for you. Sorry, I made the fix, but for some reason, failed to submit it correctly. Thanks for making the change yourself.
(In reply to comment #17) > Sorry, I made the fix, but for some reason, failed to submit it correctly. > Thanks for making the change yourself. No problem! Thanks for the fix. I'm testing the compilation here. If you want to land this sooner, you can always post a new patch and I can use the commit queue.
Committed r130348: <http://trac.webkit.org/changeset/130348>
Martin, Arno - thanks for landing this one!