Bug 68238 – [soup] Crash while loading http://www.jusco.cn

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Nayan Kumar K]

GtkLauncher crashes while loading http://www.jusco.cn with following backtrace,


Program received signal SIGSEGV, Segmentation fault.
0x00007ffff524e2a0 in WTF::OwnPtr<WebCore::ApplicationCacheHost>::get (
    this=0x8f0) at ../../Source/JavaScriptCore/wtf/OwnPtr.h:56
56	        PtrType get() const { return m_ptr; }
(gdb) bt
#0  0x00007ffff524e2a0 in WTF::OwnPtr<WebCore::ApplicationCacheHost>::get (
    this=0x8f0) at ../../Source/JavaScriptCore/wtf/OwnPtr.h:56
#1  0x00007ffff524e116 in WebCore::DocumentLoader::applicationCacheHost (this=
    0x0) at ../../Source/WebCore/loader/DocumentLoader.h:254
#2  0x00007ffff5433509 in WebCore::FrameLoader::loadResourceSynchronously (
    this=0xbea588, request=..., 
    storedCredentials=WebCore::AllowStoredCredentials, error=..., 
    response=..., data=...) at ../../Source/WebCore/loader/FrameLoader.cpp:2644
#3  0x00007ffff5423f3d in WebCore::DocumentThreadableLoader::loadRequest (
    this=0xe40c10, request=..., securityCheck=WebCore::DoSecurityCheck)
    at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:364
#4  0x00007ffff5422259 in WebCore::DocumentThreadableLoader::DocumentThreadableLoader (this=0xe40c10, document=0xcb6660, client=0xe40508, 
    blockingBehavior=WebCore::DocumentThreadableLoader::LoadSynchronously, 
    request=..., options=...)
    at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:87
#5  0x00007ffff5421e2a in WebCore::DocumentThreadableLoader::loadResourceSynchronously (document=0xcb6660, request=..., client=..., options=...)
    at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:59
#6  0x00007ffff547bf11 in WebCore::ThreadableLoader::loadResourceSynchronously
    (context=0xcb67f8, request=..., client=..., options=...)
    at ../../Source/WebCore/loader/ThreadableLoader.cpp:69
#7  0x00007ffff59be6da in WebCore::XMLHttpRequest::createRequest (
---Type <return> to continue, or q <return> to quit---
    this=0xe404e0, ec=@0x7fffffffa13c)
    at ../../Source/WebCore/xml/XMLHttpRequest.cpp:669
#8  0x00007ffff59bdb35 in WebCore::XMLHttpRequest::send (this=0xe404e0, 
    body=..., ec=@0x7fffffffa13c)
    at ../../Source/WebCore/xml/XMLHttpRequest.cpp:538
#9  0x00007ffff59bd56b in WebCore::XMLHttpRequest::send (this=0xe404e0, 
    ec=@0x7fffffffa13c) at ../../Source/WebCore/xml/XMLHttpRequest.cpp:478
#10 0x00007ffff4f37406 in WebCore::JSXMLHttpRequest::send (this=
    0x7ffff7e06be0, exec=0x7fff9e926300)
    at ../../Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:121
#11 0x00007ffff5c26b1f in WebCore::jsXMLHttpRequestPrototypeFunctionSend (
    exec=0x7fff9e926300) at DerivedSources/WebCore/JSXMLHttpRequest.cpp:518
#12 0x00007fffa7f201f8 in ?? ()
#13 0x00007fffffffa250 in ?? ()
#14 0x00007fffa7f223d5 in ?? ()
#15 0x00007fffffffa1d0 in ?? ()
#16 0x00007ffff7e06be0 in ?? ()
#17 0x0000000000cc29c0 in ?? ()
#18 0x00007ffff7e07e60 in ?? ()
#19 0x0000000000000000 in ?? ()

[Nayan Kumar K]

Crash is happening the following code, and it seems to happen for AJAX resource requests.


#if ENABLE(OFFLINE_WEB_APPLICATIONS)
        if (!documentLoader()->applicationCacheHost()->maybeLoadSynchronously(newRequest, error, response, data)) {
#endif
            ResourceHandle::loadResourceSynchronously(networkingContext(), newRequest, storedCredentials, error, response, data);
#if ENABLE(OFFLINE_WEB_APPLICATIONS)
            documentLoader()->applicationCacheHost()->maybeLoadFallbackSynchronously(newRequest, error, response, data);
        }
#endif
    }
    int encodedDataLength = response.resourceLoadInfo() ? static_cast<int>(response.resourceLoadInfo()->encodedDataLength) : -1;
        notifier()->sendRemainingDelegateMessages(m_documentLoader.get(), identifier, response, data.data(), data.size(), encodedDataLength, error);

When doing ResourceHandle::loadResourceSynchronously, it happens that, resource loading completes and documentLoader becomes NULL before reaching the line next to ResourceHandle::loadResourceSynchronously, which seems to be valid since this is a synchronous resource request. 

Added some debugging statements before and after this call and it proves that documentLoader is getting set to 0 before returning from ResourceHandle::loadResourceSynchronously. 

Given this, it make sense to guard documentLoader de-referencing with a NULL check after calling ResourceHandle::loadResourceSynchronously.

[Nayan Kumar K]

Created attachment 107633 [details]
Patch to fix crash

[Nayan Kumar K]

This crash happens either while loading http://www.jusco.cn or by loading http://www.jusco.cn, navigating to some link, pressing 'Back' button and then closing GtkLauncher. And it seem to happen only for AJAX resource requests (like the one in  http://www.jusco.cn). It is highly difficult to add a layout test to simulate this use case!

[Darin Adler]

Comment on attachment 107633 [details]
Patch to fix crash

View in context: https://bugs.webkit.org/attachment.cgi?id=107633&action=review

> Source/WebCore/ChangeLog:12
> +        No new tests. (OOPS!)

Tests are required for WebKit bug fixes. Could you please create a regression test demonstrating this problem?

[Darin Adler]

Comment on attachment 107633 [details]
Patch to fix crash

View in context: https://bugs.webkit.org/attachment.cgi?id=107633&action=review

>> Source/WebCore/ChangeLog:12
>> +        No new tests. (OOPS!)
> 
> Tests are required for WebKit bug fixes. Could you please create a regression test demonstrating this problem?

If you are not going to supply a test you need to explain why, here. If you just leave in the OOPS, then we can’t check the patch in. The pre-commit script will reject it.

[Nayan Kumar K]

Created attachment 107694 [details]
CrashFix

[Nayan Kumar K]

(In reply to comment #5)
> (From update of attachment 107633 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107633&action=review
> 
> >> Source/WebCore/ChangeLog:12
> >> +        No new tests. (OOPS!)
> > 
> > Tests are required for WebKit bug fixes. Could you please create a regression test demonstrating this problem?
> 
> If you are not going to supply a test you need to explain why, here. If you just leave in the OOPS, then we can’t check the patch in. The pre-commit script will reject it.

I have seen this crash happening only with http://www.jusco.cn. This is a random crash, sometimes seen just when we open this site. Sometimes we will have to navigate to some link from this page and press back and then close the GtkLuancher to make GtkLauncher crash. I am not sure whether it can be simulated via a LayoutTests. 

Hence marked the "tests" field in ChangeLog as "No tests added"

[Darin Adler]

Comment on attachment 107694 [details]
CrashFix

View in context: https://bugs.webkit.org/attachment.cgi?id=107694&action=review

> Source/WebCore/loader/FrameLoader.cpp:2651
>      int encodedDataLength = response.resourceLoadInfo() ? static_cast<int>(response.resourceLoadInfo()->encodedDataLength) : -1;
> -    notifier()->sendRemainingDelegateMessages(m_documentLoader.get(), identifier, response, data.data(), data.size(), encodedDataLength, error);
> +    if (documentLoader())
> +        notifier()->sendRemainingDelegateMessages(m_documentLoader.get(), identifier, response, data.data(), data.size(), encodedDataLength, error);

Given that the next line of code uses m_documentLoader.get() it is not great style to use documentLoader() on the line before. It should be:

    if (m_documentLoader)

I also am unclear that not sending a delegate message at all is the right thing to do at this level. Is there anything useful sendRemainingDelegateMessages can do without a document loader?

It also seems that the encodedDataLength computation should be inside the if statement, not outside.

[Nayan Kumar K]

(In reply to comment #8)
> (From update of attachment 107694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107694&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:2651
> >      int encodedDataLength = response.resourceLoadInfo() ? static_cast<int>(response.resourceLoadInfo()->encodedDataLength) : -1;
> > -    notifier()->sendRemainingDelegateMessages(m_documentLoader.get(), identifier, response, data.data(), data.size(), encodedDataLength, error);
> > +    if (documentLoader())
> > +        notifier()->sendRemainingDelegateMessages(m_documentLoader.get(), identifier, response, data.data(), data.size(), encodedDataLength, error);
> 
> Given that the next line of code uses m_documentLoader.get() it is not great style to use documentLoader() on the line before. It should be:
> 
>     if (m_documentLoader)
Sure, will do.
> 
> I also am unclear that not sending a delegate message at all is the right thing to do at this level. Is there anything useful sendRemainingDelegateMessages can do without a document loader?

If we call sendRemainingDelegateMessage, in-spite DocumentLoader being NULL, it crashs here,

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff6acfc1e in WebKit::FrameLoaderClient::dispatchDidReceiveResponse(WebCore::DocumentLoader*, unsigned long, WebCore::ResourceResponse const&) ()
   from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
(gdb) bt
#0  0x00007ffff6acfc1e in WebKit::FrameLoaderClient::dispatchDidReceiveResponse(WebCore::DocumentLoader*, unsigned long, WebCore::ResourceResponse const&) ()
   from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#1  0x00007ffff7010d42 in WebCore::ResourceLoadNotifier::dispatchDidReceiveResponse(WebCore::DocumentLoader*, unsigned long, WebCore::ResourceResponse const&) ()
   from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#2  0x00007ffff7011152 in WebCore::ResourceLoadNotifier::sendRemainingDelegateMessages(WebCore::DocumentLoader*, unsigned long, WebCore::ResourceResponse const&, char const*, int, int, WebCore::ResourceError const&) () from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#3  0x00007ffff6fdc182 in WebCore::FrameLoader::loadResourceSynchronously(WebCore::ResourceRequest const&, WebCore::StoredCredentials, WebCore::ResourceError&, WebCore::ResourceResponse&, WTF::Vector<char, 0ul>&) () from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#4  0x00007ffff6fd0f17 in WebCore::DocumentThreadableLoader::loadRequest(WebCore::ResourceRequest const&, WebCore::SecurityCheckPolicy) ()
   from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#5  0x00007ffff6fd2528 in WebCore::DocumentThreadableLoader::DocumentThreadableLoader(WebCore::Document*, WebCore::ThreadableLoaderClient*, WebCore::DocumentThreadableLoader::BlockingBehavior, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderOptions const&) ()
   from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#6  0x00007ffff6fd27cc in WebCore::DocumentThreadableLoader::loadResourceSynchronously(WebCore::Document*, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderClient&, WebCore::ThreadableLoaderOptions const&) () from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#7  0x00007ffff744e327 in WebCore::XMLHttpRequest::createRequest(int&) () from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#8  0x00007ffff744ee9b in WebCore::XMLHttpRequest::send(WTF::String const&, int&) () from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#9  0x00007ffff744f358 in WebCore::XMLHttpRequest::send(int&) () from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#10 0x00007ffff6c34764 in WebCore::JSXMLHttpRequest::send(JSC::ExecState*) () from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#11 0x00007ffff7708ceb in WebCore::jsXMLHttpRequestPrototypeFunctionSend(JSC::ExecState*) ()
   from /home/nayankk/Sources/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0

This is because, we try to de-reference documentLoader() in FrameLoaderClient::dispatchDidReceievResponse function. Clients assume that, these callbacks will not be called with NULL documentLoader(), hence it makes sense not to send trigger sending callbacks. Also,  sendRemainingDelegateMessages does nothing extra than triggering these client callbacks.

> 
> It also seems that the encodedDataLength computation should be inside the if statement, not outside.
Sure.

[Nayan Kumar K]

Created attachment 107701 [details]
CrashFix

[Alexey Proskuryakov]

Comment on attachment 107701 [details]
CrashFix

View in context: https://bugs.webkit.org/attachment.cgi?id=107701&action=review

> Source/WebCore/ChangeLog:13
> +        No tests added, since this is a random crash happening only with http://www.jusco.cn, upon 
> +        loading, navigating back and closing in GtkLauncher.

I don't think that this is a sufficient explanation. Per the description, it does happen somewhat reproducibly, and it certainly doesn't matter how many sites are affected.

Do you know if this happens in other WebKit ports?

[Nayan Kumar K]

(In reply to comment #11)
> (From update of attachment 107701 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107701&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        No tests added, since this is a random crash happening only with http://www.jusco.cn, upon 
> > +        loading, navigating back and closing in GtkLauncher.
> 
> I don't think that this is a sufficient explanation. Per the description, it does happen somewhat reproducibly, and it certainly doesn't matter how many sites are affected.
> 
> Do you know if this happens in other WebKit ports?

Well, I don't see crash in QtLauncher(!!). I observed that, 5 XMLHttpRequests gets triggered in both QtLauncher and GtkLauncher. While, in QtLauncher, these request are made "one after the other", i.e., new request is made after getting the response for previous request, whereas, in GtkLuancher, all 5 requests are triggered at once, and then we start receiving response. Interestingly, GtkLauncher processed the requests one-after-the-other for a a reduced test case which triggers 5 XMLHttpRequest at once!.

[Nayan Kumar K]

Some more analysis reveals the following,

1). The page http://www.jusco.cn has 6 iframes, 5 of which triggers XMLHttpRequest (e.g, http://m.weather.com.cn/m/p2/weather1.htm?id=101280101T)

2). During the loading of the page, these XMLHttpRequest gets triggered many times. Code flow is

XMLHttpRequest->ThreadableLoader->DocumentThreadableLoader->FrameLoader->ResourceHandle->ResourceHandleSoup

Note that documentLoader doesn't get involved in this code path.

3). ResourceHandleSoup triggers the synchronous network request and waits for the response,

#39 0x00007ffff0c2a9f2 in g_main_loop_run (loop=0x7fff8c051220) at /build/buildd/glib2.0-2.28.6/./glib/gmain.c:3299
#40 0x00007ffff55f0c99 in WebCore::WebCoreSynchronousLoader::run (this=0x7fffffffbae0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:125
#41 0x00007ffff55f3c5d in WebCore::ResourceHandle::loadResourceSynchronously (context=0xb21990, request=..., error=..., response=..., data=...)
    at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:752
#42 0x00007ffff54334f2 in WebCore::FrameLoader::loadResourceSynchronously (this=0x736028, request=..., storedCredentials=WebCore::AllowStoredCredentials, error=..., response=..., data=...)
    at ../../Source/WebCore/loader/FrameLoader.cpp:2642
#43 0x00007ffff5423f3d in WebCore::DocumentThreadableLoader::loadRequest (this=0x7fff8c03fd90, request=..., securityCheck=WebCore::DoSecurityCheck)
    at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:364
---Type <return> to continue, or q <return> to quit--- 
#44 0x00007ffff5422259 in WebCore::DocumentThreadableLoader::DocumentThreadableLoader (this=0x7fff8c03fd90, document=0x7fff8c0b8300, client=0x7fff8c04fe48, 
    blockingBehavior=WebCore::DocumentThreadableLoader::LoadSynchronously, request=..., options=...) at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:87
#45 0x00007ffff5421e2a in WebCore::DocumentThreadableLoader::loadResourceSynchronously (document=0x7fff8c0b8300, request=..., client=..., options=...)
    at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:59
#46 0x00007ffff547bf11 in WebCore::ThreadableLoader::loadResourceSynchronously (context=0x7fff8c0b8498, request=..., client=..., options=...) at ../../Source/WebCore/loader/ThreadableLoader.cpp:69
#47 0x00007ffff59be6da in WebCore::XMLHttpRequest::createRequest (this=0x7fff8c04fe20, ec=@0x7fffffffc6fc) at ../../Source/WebCore/xml/XMLHttpRequest.cpp:669
#48 0x00007ffff59bdb35 in WebCore::XMLHttpRequest::send (this=0x7fff8c04fe20, body=..., ec=@0x7fffffffc6fc) at ../../Source/WebCore/xml/XMLHttpRequest.cpp:538
#49 0x00007ffff59bd56b in WebCore::XMLHttpRequest::send (this=0x7fff8c04fe20, ec=@0x7fffffffc6fc) at ../../Source/WebCore/xml/XMLHttpRequest.cpp:478
#50 0x00007ffff4f37406 in WebCore::JSXMLHttpRequest::send (this=0x7ffff7e1e1a0, exec=0x7fff9a146160) at ../../Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:121
#51 0x00007ffff5c26b1f in WebCore::jsXMLHttpRequestPrototypeFunctionSend (exec=0x7fff9a146160) at DerivedSources/WebCore/JSXMLHttpRequest.cpp:518

4). The page which got loaded in frame(http://m.weather.com.cn/m/p2/weather1.htm?id=101280101T) has the logic of appending a node via JavaScript,

B=document.createElement("script");
B.type="text/javascript";B.id="wratingSuevey";
B.src="http://tongji.wrating.com/survey/check.php?c="+vjAcc;
document.getElementsByTagName("head")[0].appendChild(B)}

5). While soup waits for response from network, webcore shared timer gets triggered and it processes the above JavaScript, which eventually results in setting of documentLoader of frame to 0, (detachViewsAndDocumentLoader sets documentLoader of frame to 0)

Breakpoint 3, WebCore::FrameLoader::setDocumentLoader (this=0x74c8e8, loader=0x0) at ../../Source/WebCore/loader/FrameLoader.cpp:1677
1677	}
(gdb) bt
#0  WebCore::FrameLoader::setDocumentLoader (this=0x74c8e8, loader=0x0) at ../../Source/WebCore/loader/FrameLoader.cpp:1677
#1  0x00007ffff5432836 in WebCore::FrameLoader::detachViewsAndDocumentLoader (this=0x74c8e8) at ../../Source/WebCore/loader/FrameLoader.cpp:2478
#2  0x00007ffff5432749 in WebCore::FrameLoader::detachFromParent (this=0x74c8e8) at ../../Source/WebCore/loader/FrameLoader.cpp:2463
#3  0x00007ffff54326b2 in WebCore::FrameLoader::frameDetached (this=0x74c8e8) at ../../Source/WebCore/loader/FrameLoader.cpp:2446
#4  0x00007ffff529b118 in WebCore::HTMLFrameOwnerElement::willRemove (this=0x74be30) at ../../Source/WebCore/html/HTMLFrameOwnerElement.cpp:58
#5  0x00007ffff529a8c3 in WebCore::HTMLFrameElementBase::willRemove (this=0x74be30) at ../../Source/WebCore/html/HTMLFrameElementBase.cpp:280
#6  0x00007ffff50bcef4 in WebCore::ContainerNode::willRemove (this=0x747710) at ../../Source/WebCore/dom/ContainerNode.cpp:379
#7  0x00007ffff511c035 in WebCore::Element::willRemove (this=0x747710) at ../../Source/WebCore/dom/Element.cpp:901
#8  0x00007ffff50bcef4 in WebCore::ContainerNode::willRemove (this=0x747ab0) at ../../Source/WebCore/dom/ContainerNode.cpp:379
#9  0x00007ffff511c035 in WebCore::Element::willRemove (this=0x747ab0) at ../../Source/WebCore/dom/Element.cpp:901
#10 0x00007ffff50bcfba in WebCore::willRemoveChild (child=0x747ab0) at ../../Source/WebCore/dom/ContainerNode.cpp:392
#11 0x00007ffff50bd19a in WebCore::ContainerNode::removeChild (this=0x72cda0, oldChild=0x747ab0, ec=@0x7fffffffb2a8) at ../../Source/WebCore/dom/ContainerNode.cpp:432
#12 0x00007ffff50bdb82 in WebCore::ContainerNode::appendChild (this=0x72cda0, newChild=..., ec=@0x7fffffffb2a8, shouldLazyAttach=true) at ../../Source/WebCore/dom/ContainerNode.cpp:620
#13 0x00007ffff5141c25 in WebCore::Node::appendChild (this=0x72cda0, newChild=..., ec=@0x7fffffffb2a8, shouldLazyAttach=true) at ../../Source/WebCore/dom/Node.cpp:683
#14 0x00007ffff4f1cef9 in WebCore::JSNode::appendChild (this=0x7fff9a13aae0, exec=0x7fff9a146230) at ../../Source/WebCore/bindings/js/JSNodeCustom.cpp:183
#15 0x00007ffff5baaa9a in WebCore::jsNodePrototypeFunctionAppendChild (exec=0x7fff9a146230) at DerivedSources/WebCore/JSNode.cpp:512
#16 0x00007fffa7f201f8 in ?? ()
#17 0x00007fffffffb3b0 in ?? ()
#18 0x00007fffa7f21924 in ?? ()
#19 0x00007fffffffb330 in ?? ()
#20 0x00007fff9a13a720 in ?? ()
#21 0xffff000000000000 in ?? ()
#22 0xffff000000000007 in ?? ()
#23 0x00007fffa7f212e1 in ?? ()
#24 0x00007ffff3f1daf7 in JSC::Register::Register (this=0x7ffff0ed4630) at ../../Source/JavaScriptCore/interpreter/Register.h:93
#25 0x00007ffff3fa1988 in JSC::JITCode::execute (this=0x7ffff7e160b8, registerFile=0x4e8628, callFrame=0x7fff9a1461d8, globalData=0x4e52f0) at ../../Source/JavaScriptCore/jit/JITCode.h:107
#26 0x00007ffff3f9e186 in JSC::Interpreter::executeCall (this=0x4e8610, callFrame=0x7fff9a133d28, function=0x7ffff7e1e020, callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...)
    at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:971
#27 0x00007ffff4039f9f in JSC::call (exec=0x7fff9a133d28, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../Source/JavaScriptCore/runtime/CallData.cpp:39
#28 0x00007ffff4ed2d1f in WebCore::JSMainThreadExecState::call (exec=0x7fff9a133d28, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...)
    at ../../Source/WebCore/bindings/js/JSMainThreadExecState.h:52
#29 0x00007ffff4f40314 in WebCore::ScheduledAction::executeFunctionInContext (this=0x8ad580, globalObject=0x7fff9a133ca0, thisValue=..., context=0x67cc98)
    at ../../Source/WebCore/bindings/js/ScheduledAction.cpp:110
#30 0x00007ffff4f404da in WebCore::ScheduledAction::execute (this=0x8ad580, document=0x67cb00) at ../../Source/WebCore/bindings/js/ScheduledAction.cpp:130
#31 0x00007ffff4f400e4 in WebCore::ScheduledAction::execute (this=0x8ad580, context=0x67cc98) at ../../Source/WebCore/bindings/js/ScheduledAction.cpp:80
#32 0x00007ffff54b70d7 in WebCore::DOMTimer::fired (this=0x88ddc0) at ../../Source/WebCore/page/DOMTimer.cpp:135
#33 0x00007ffff5626833 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x4ed4b0) at ../../Source/WebCore/platform/ThreadTimers.cpp:115
#34 0x00007ffff5626761 in WebCore::ThreadTimers::sharedTimerFired () at ../../Source/WebCore/platform/ThreadTimers.cpp:93
#35 0x00007ffff5da7f5e in WebCore::timeout_cb () at ../../Source/WebCore/platform/gtk/SharedTimerGtk.cpp:49
#36 0x00007ffff0c2b4eb in g_timeout_dispatch (source=0x957320, callback=<value optimized out>, user_data=<value optimized out>) at /build/buildd/glib2.0-2.28.6/./glib/gmain.c:3882
#37 0x00007ffff0c29bcd in g_main_dispatch (context=0xffff000000000002) at /build/buildd/glib2.0-2.28.6/./glib/gmain.c:2440
#38 g_main_context_dispatch (context=0xffff000000000002) at /build/buildd/glib2.0-2.28.6/./glib/gmain.c:3013
#39 0x00007ffff0c2a3a8 in g_main_context_iterate (context=0x4364f0, block=<value optimized out>, dispatch=1, self=<value optimized out>) at /build/buildd/glib2.0-2.28.6/./glib/gmain.c:3091
#40 0x00007ffff0c2a9f2 in g_main_loop_run (loop=0x7fff8c051220) at /build/buildd/glib2.0-2.28.6/./glib/gmain.c:3299
#41 0x00007ffff55f0c99 in WebCore::WebCoreSynchronousLoader::run (this=0x7fffffffbae0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:125
#42 0x00007ffff55f3c5d in WebCore::ResourceHandle::loadResourceSynchronously (context=0xb21990, request=..., error=..., response=..., data=...)
    at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:752
#43 0x00007ffff54334f2 in WebCore::FrameLoader::loadResourceSynchronously (this=0x736028, request=..., storedCredentials=WebCore::AllowStoredCredentials, error=..., response=..., data=...)
    at ../../Source/WebCore/loader/FrameLoader.cpp:2642
#44 0x00007ffff5423f3d in WebCore::DocumentThreadableLoader::loadRequest (this=0x7fff8c03fd90, request=..., securityCheck=WebCore::DoSecurityCheck)
---Type <return> to continue, or q <return> to quit---
    at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:364
#45 0x00007ffff5422259 in WebCore::DocumentThreadableLoader::DocumentThreadableLoader (this=0x7fff8c03fd90, document=0x7fff8c0b8300, client=0x7fff8c04fe48, 
    blockingBehavior=WebCore::DocumentThreadableLoader::LoadSynchronously, request=..., options=...) at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:87
#46 0x00007ffff5421e2a in WebCore::DocumentThreadableLoader::loadResourceSynchronously (document=0x7fff8c0b8300, request=..., client=..., options=...)
    at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:59
#47 0x00007ffff547bf11 in WebCore::ThreadableLoader::loadResourceSynchronously (context=0x7fff8c0b8498, request=..., client=..., options=...) at ../../Source/WebCore/loader/ThreadableLoader.cpp:69
#48 0x00007ffff59be6da in WebCore::XMLHttpRequest::createRequest (this=0x7fff8c04fe20, ec=@0x7fffffffc6fc) at ../../Source/WebCore/xml/XMLHttpRequest.cpp:669
#49 0x00007ffff59bdb35 in WebCore::XMLHttpRequest::send (this=0x7fff8c04fe20, body=..., ec=@0x7fffffffc6fc) at ../../Source/WebCore/xml/XMLHttpRequest.cpp:538
#50 0x00007ffff59bd56b in WebCore::XMLHttpRequest::send (this=0x7fff8c04fe20, ec=@0x7fffffffc6fc) at ../../Source/WebCore/xml/XMLHttpRequest.cpp:478
#51 0x00007ffff4f37406 in WebCore::JSXMLHttpRequest::send (this=0x7ffff7e1e1a0, exec=0x7fff9a146160) at ../../Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:121
#52 0x00007ffff5c26b1f in WebCore::jsXMLHttpRequestPrototypeFunctionSend (exec=0x7fff9a146160) at DerivedSources/WebCore/JSXMLHttpRequest.cpp:518
#53 0x00007fffa7f201f8 in ?? ()
#54 0x00007fffffffc810 in ?? ()
#55 0x00007fffe7f1fe54 in ?? ()
#56 0x00007fffffffc790 in ?? ()
#57 0x00007ffff7e1e1a0 in ?? ()
#58 0x00007fff8c081e40 in ?? ()
#59 0x00007ffff7e2fae0 in ?? ()
#60 0x0000000000000000 in ?? ()
(gdb)

6). Once soup gets response from network, it returns and find that documentLoader is 0 and crashes!.


Program received signal SIGSEGV, Segmentation fault.
0x00007ffff524e2a0 in WTF::OwnPtr<WebCore::ApplicationCacheHost>::get (
    this=0x8f0) at ../../Source/JavaScriptCore/wtf/OwnPtr.h:56
56            PtrType get() const { return m_ptr; }
(gdb) bt
#0  0x00007ffff524e2a0 in WTF::OwnPtr<WebCore::ApplicationCacheHost>::get (
    this=0x8f0) at ../../Source/JavaScriptCore/wtf/OwnPtr.h:56
#1  0x00007ffff524e116 in WebCore::DocumentLoader::applicationCacheHost (this=
    0x0) at ../../Source/WebCore/loader/DocumentLoader.h:254
#2  0x00007ffff5433509 in WebCore::FrameLoader::loadResourceSynchronously (
    this=0xbea588, request=..., 
    storedCredentials=WebCore::AllowStoredCredentials, error=..., 
    response=..., data=...) at ../../Source/WebCore/loader/FrameLoader.cpp:2644
#3  0x00007ffff5423f3d in WebCore::DocumentThreadableLoader::loadRequest (
    this=0xe40c10, request=..., securityCheck=WebCore::DoSecurityCheck)
    at ../../Source/WebCore/loader/DocumentThreadableLoader.cpp:364
#4  0x00007ffff5422259 in WebCore::DocumentThreadableLoader::DocumentThreadableLoader (this=0xe40c10, document=0xcb6660, client=0xe40508, 
    blockingBehavior=WebCore::DocumentThreadableLoader::LoadSynchronously, 
    request=..., options=...)
...

This seems to be a complex use case to simulate via a layout test, as there are many iframes, XHR requests and dynamic update of DOM involved in this scenario. It also depends on network response time, as if soup gets response quickly, it might not allow shared webcore time to timeout. I tried simulating this behavior with a layout test with no luck. :(

[Nayan Kumar K]

Created attachment 108476 [details]
JuscoCrashFix

[Alexey Proskuryakov]

Comment on attachment 108476 [details]
JuscoCrashFix

> 5). While soup waits for response from network, webcore shared timer gets triggered and it processes the above JavaScript

This is the bug in Soup back-end that needs to be fixed. Timers should not fire during synchronous XMLHttpRequest. On other platforms, this is achieved by running a nested event loop in a mode that doesn't handle any events except for this particular load's network callbacks. E.g. in ResourceHandleCFNet.cpp:

    CFURLConnectionScheduleWithRunLoop(handle->connection(), CFRunLoopGetCurrent(), synchronousLoadRunLoopMode());
...

    while (!client->isDone())
        CFRunLoopRunInMode(synchronousLoadRunLoopMode(), UINT_MAX, true);

[Alexey Proskuryakov]

To clarify, I'm not saying that this is necessarily a bug in Soup. WebCoreSynchronousLoader::run() in ResourceHandleSoup.cpp is wrong to just run main event loop, but I don't now if Soup provides the ability to do loading in a custom run loop mode.

[Nayan Kumar K]

(In reply to comment #16)
> To clarify, I'm not saying that this is necessarily a bug in Soup. WebCoreSynchronousLoader::run() in ResourceHandleSoup.cpp is wrong to just run main event loop, but I don't now if Soup provides the ability to do loading in a custom run loop mode.

Currently WebSynchronousLoader seems to be simulating synchronous loading behavior via SoupSessionAsync. May be we need to do use SoupSessionSync for XMLHttpRequests? Martin?

[Martin Robinson]

Looks like this issue is also causing fast/files/apply-blob-url-to-xhr.html to crash in the DRT. See https://bugs.webkit.org/show_bug.cgi?id=54234

[Martin Robinson]

*** Bug 54234 has been marked as a duplicate of this bug. ***

[Martin Robinson]

I think that the right thing to do here is to push a new GMainContext as the thread default during synchronous loads. This problem was interesting and has been at the back of my mind lately, so I wrote a test and patch.

[Martin Robinson]

Created attachment 109450 [details]
Patch

[WebKit Review Bot]

Attachment 109450 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:880:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

(In reply to comment #22)

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:880:  Use 0 instead of NULL.  [readability/null] [5]
> Total errors found: 1 in 8 files

This is a false positive, because g_object_set requires the use of NULL in place of 0 for sentinels.

[Collabora GTK+ EWS bot]

Comment on attachment 109450 [details]
Patch

Attachment 109450 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9930303

[Xan Lopez]

I think an informal review by Dan would be useful here.

[Martin Robinson]

Comment on attachment 109450 [details]
Patch

Clearing review flag, because this patch will be a lot simpler once some fixes in libsoup master make it to release.

[Martin Robinson]

Created attachment 118510 [details]
Patch that doesn't work

Dan, I've uploaded a patch that doesn't seem to be working. It uses the new 'use-thread-context' property in Soup, but the request never completes.

[Dan Winship]

(In reply to comment #27)
> Created an attachment (id=118510) [details]
> Patch that doesn't work
> 
> Dan, I've uploaded a patch that doesn't seem to be working. It uses the new 'use-thread-context' property in Soup, but the request never completes.

So this got tracked down to a bug in libsoup that is fixed as of 2.37.5.

Bumping the libsoup requirement up would also let us land bug 42432 (Web Timing). (Current release is 2.37.90, though we'd probably want to wait for 2.37.91 next week to fix a few bugs in 2.37.90.)

[Dan Winship]

OK, a problem with the patch: readCallback() calls client->didFinish() and then asynchronously closes the connection. But the sync loader stops running its main loop after didFinish() is called, and so the async close never gets to finish.

Moving the didFinish() into closeCallback() would be the obvious fix.

[Martin Robinson]

Created attachment 129994 [details]
New patch

[Dan Winship]

Comment on attachment 129994 [details]
New patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129994&action=review

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:99
>      m_mainLoop = adoptGRef(g_main_loop_new(0, false));

mismerge while rebasing; that line should have gone away
(and presumably the newly-added m_mainLoop assignment should get the adoptGRef?)

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:134
> +    // We quit the main loop asynchronously here, because didFinishLoading is called before
> +    // the closeCallback. If we quit the main loop before closeCallback, it will never be
> +    // called because the synchronous thread context is no longer active.

This assumes that the close will finish before the next iteration of the main loop. Which is currently true for SoupHTTPInputStream, but I don't think we want to guarantee that forever. (And if we could, then we wouldn't really need to use close_async() anyway, we could just do a sync close().)

Maybe call didFinishLoading from readCallback for async loads, and from closeCallback for sync loads?

[Martin Robinson]

Created attachment 130004 [details]
Updated patch addressin Dan's comments

[Martin Robinson]

(In reply to comment #31)

Thanks for the review! I've uploaded a new patch that I hope addresses your comments fully.

> (From update of attachment 129994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129994&action=review
> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:99
> >      m_mainLoop = adoptGRef(g_main_loop_new(0, false));
> 
> mismerge while rebasing; that line should have gone away
> (and presumably the newly-added m_mainLoop assignment should get the adoptGRef?)

Nice catch.

> Maybe call didFinishLoading from readCallback for async loads, and from closeCallback for sync loads?

I've done precisely this now.

[WebKit Review Bot]

Comment on attachment 130004 [details]
Updated patch addressin Dan's comments

Attachment 130004 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11796419

New failing tests:
editing/selection/select-line-break-with-opposite-directionality.html

[Martin Robinson]

(In reply to comment #34)
> (From update of attachment 130004 [details])
> Attachment 130004 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11796419
> 
> New failing tests:
> editing/selection/select-line-break-with-opposite-directionality.html

This failure seems unrelated.

[Carlos Garcia Campos]

Comment on attachment 130004 [details]
Updated patch addressin Dan's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=130004&action=review

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:87
> +        // We don't want any timers to fire while we are doing our synchronous load
> +        // so we replace the thread default main context. The main loop iterations
> +        // will only process GSources associated with this inner context.
> +        GRefPtr<GMainContext> innerMainContext = adoptGRef(g_main_context_new());
> +        g_main_context_push_thread_default(innerMainContext.get());
> +        m_mainLoop = g_main_loop_new(innerMainContext.get(), false);

I think should call push_thread_default right before running the loop, instead of before creating it.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:92
> +        g_main_context_pop_thread_default(g_main_context_get_thread_default());

You should pass the context you pushed to make sure you are removing the context you want.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:112
> +        g_main_loop_quit(m_mainLoop.get());

You could check loop is actually running with g_main_loop_is_running(), just in case this method is called twice or before run().

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:124
> +        if (!m_finished)

You could use g_main_loop_is_running() so that you don't even need m_finished in the class.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:125
> +            g_main_loop_run(m_mainLoop.get());

I would push and pop the context here.

[Dan Winship]

(In reply to comment #36)
> I think should call push_thread_default right before running the loop, instead of before creating it.

That won't work; it has to have been pushed before loadResourceSynchronously() creates the ResourceHandle, so that it's the thread-default context at the time when the SoupMessage is queued.

But yeah, the way the inner context gets handled here is a little bit magical and fragile... another possibility would be to have loadResourceSynchronously() create and push the context itself, and then pass it to the WebCoreSynchronousLoader, (and pop it after calling .run()), so that then it's more explicit exactly when the new context is in effect.

[Martin Robinson]

Comment on attachment 130004 [details]
Updated patch addressin Dan's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=130004&action=review

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:87
>> +        m_mainLoop = g_main_loop_new(innerMainContext.get(), false);
> 
> I think should call push_thread_default right before running the loop, instead of before creating it.

I believe it's important to push the context before soup is invoked, so we need to push it here.

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:92
>> +        g_main_context_pop_thread_default(g_main_context_get_thread_default());
> 
> You should pass the context you pushed to make sure you are removing the context you want.

Synchronous loaders are never nested. I'm not sure such an assertion brings much value here.

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:112
>> +        g_main_loop_quit(m_mainLoop.get());
> 
> You could check loop is actually running with g_main_loop_is_running(), just in case this method is called twice or before run().

Okay. I can add this check here. My patch originally didn't touch this code at all, so it didn't make sense to fix other potential bugs. This is a reasonable check though.

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:124
>> +        if (!m_finished)
> 
> You could use g_main_loop_is_running() so that you don't even need m_finished in the class.

I'm not sure how that could prevent using m_finished. The check here prevents the main loop from starting if the networking has finished before ::run is called.

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:125
>> +            g_main_loop_run(m_mainLoop.get());
> 
> I would push and pop the context here.

As I said above, that probably won't work.

[Martin Robinson]

(In reply to comment #37)

> But yeah, the way the inner context gets handled here is a little bit magical and fragile... another possibility would be to have loadResourceSynchronously() create and push the context itself, and then pass it to the WebCoreSynchronousLoader, (and pop it after calling .run()), so that then it's more explicit exactly when the new context is in effect.

I agree that it's a bit fragile. We do have a guarantee that it happens before the load start, because the client must be instatiated before a load operation begins.

[Martin Robinson]

Committed r109760: <http://trac.webkit.org/changeset/109760>

[Martin Robinson]

Reopening to attach new patch.

[Martin Robinson]

Created attachment 130289 [details]
Patch fixing segfault

[Martin Robinson]

(In reply to comment #42)
> Created an attachment (id=130289) [details]
> Patch fixing segfault

It seems that calling didFinishLoad can make the client invalid, without setting it to null. So here I just set the client to null manually after call didFinishLoad.

[Philippe Normand]

Comment on attachment 130289 [details]
Patch fixing segfault

View in context: https://bugs.webkit.org/attachment.cgi?id=130289&action=review

Thanks! Just a little nit:

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:683
> +            handle->setClient(0); // Unset the client so that we do not try to access th
> +                                  // client in the closeCallback.

Maybe the comment can fit in the line above the setClient call? Just a suggestion to avoid repeating "the client":

// Unset the client so that we do not try to access it in the closeCallback.

[Martin Robinson]

It seems a bug in libsoup causes many followup tests to time out so this must wait until libsoup 2.37.92.

[Martin Robinson]

Comment on attachment 130289 [details]
Patch fixing segfault

Clearing review flag for the moment. :(

[Alexey Proskuryakov]

Comment on attachment 130289 [details]
Patch fixing segfault

View in context: https://bugs.webkit.org/attachment.cgi?id=130289&action=review

> Source/WebCore/platform/network/ResourceHandleClient.h:113
> +        virtual bool isSynchronousClient() { return false; }

Why does this need to be on ResourceHandleClient? Every ResourceHandle already knows whether it's synchronous or not, because different methods are called in these cases. It doesn't need to ask the client.

[Martin Robinson]

(In reply to comment #47)
> (From update of attachment 130289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130289&action=review
> 
> > Source/WebCore/platform/network/ResourceHandleClient.h:113
> > +        virtual bool isSynchronousClient() { return false; }
> 
> Why does this need to be on ResourceHandleClient? Every ResourceHandle already knows whether it's synchronous or not, because different methods are called in these cases. It doesn't need to ask the client.

Is that the case? For libsoup at least, the path seems to be:

1. static ResourceHandle::loadResourceSynchronously
2. static ResourceHandle::create (factory method)
3. ResourceHandle::start

Does this differ greatly from the asynchronous path? I could very well be missing it here.

 I'd be happy to add whether or not the handle is synchronous as an argument to ResourceHandle::create and keep the logic out of ResourceHandleClient. Another option would be to rename ResourceHandleClient::isSynchronousClient() to ResourceHandleClient::shouldFinishLoadAfterConnectionClose(), which better matches the constraints of this particular client.

[Alexey Proskuryakov]

So far, it's the same code path on Mac, yes. I think that the difference is that on Mac, synchronous loader client just implements stopping run loop internally, not via exposing a flag to caller:

void WebCoreSynchronousLoaderClient::didFinishLoading(ResourceHandle*, double)
{
    m_isDone = true;
}

Another approach I can suggest is to just set a member variable after calling ResourceHandle::create:

    RefPtr<ResourceHandle> handle = adoptRef(new ResourceHandle(...));
    handle->m_isLoadingSynchronously = true;


Making client interface fatter creates a long term liability - everyone doing any ResourceHandle refactoring will need to spend time understanding what it is about.

[Martin Robinson]

Created attachment 136038 [details]
Patch

[Martin Robinson]

(In reply to comment #49)
> Another approach I can suggest is to just set a member variable after calling ResourceHandle::create:
> 
>     RefPtr<ResourceHandle> handle = adoptRef(new ResourceHandle(...));
>     handle->m_isLoadingSynchronously = true;

There can be only one synchronous XMLHttpRequest at a time in WebCore, so I chose to use a global variable specific to the soup implementation. This avoids adding any new code to platform independent files and the code is located more closely to the areas that need it.

[Alexey Proskuryakov]

Thank you, makes sense.

[WebKit Review Bot]

Comment on attachment 136038 [details]
Patch

Clearing flags on attachment: 136038

Committed r113604: <http://trac.webkit.org/changeset/113604>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sergio Villar Senin]

This patch (or the associated libsoup changes) are causing a lot of hangs (the main loop iterates forever) when issuing synchronous requests. For example try to load this URL: http://www.datamation.com/applications/how-libreoffice-writer-tops-ms-word-12-features-1.html