Bug 102394 - [Soup] Null-checking is required in cookiesEnabled
Summary: [Soup] Null-checking is required in cookiesEnabled
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-15 08:30 PST by ChangSeok Oh
Modified: 2017-03-11 10:54 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.75 KB, patch)
2012-11-15 08:40 PST, ChangSeok Oh
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ChangSeok Oh 2012-11-15 08:30:36 PST
CookieJarSoup.cpp is revised in this bug, https://bugs.webkit.org/show_bug.cgi?id=101621

After that, we may face a crash if NetworkingContext is null for any reasons in cookieEnabled() at CookieJarSoup.cpp
No checking context will cause a crash in cookieJarForContext because of context->soupSession().
Comment 1 ChangSeok Oh 2012-11-15 08:40:46 PST
Created attachment 174455 [details]
Patch
Comment 2 Martin Robinson 2012-11-15 08:44:03 PST
Does this fix an actual crash? If so, a test case is generally required.
Comment 3 Alexey Proskuryakov 2012-11-15 08:45:07 PST
Comment on attachment 174455 [details]
Patch

What are the reasons for context to be null? If that happens, it's possibly a bug at call site.

Can you post a stack trace for the crash?
Comment 4 ChangSeok Oh 2012-11-15 08:48:50 PST
@mrobinson. yes the crash is gone with this change.

The callstack is..

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff46efab3 in WebCore::cookieJarForContext (context=0x0)
    at ../../Source/WebCore/platform/network/soup/CookieJarSoup.cpp:37
37	    return SOUP_COOKIE_JAR(soup_session_get_feature(context->soupSession(), SOUP_TYPE_COOKIE_JAR));
(gdb) bt
#0  0x00007ffff46efab3 in WebCore::cookieJarForContext (context=0x0)
    at ../../Source/WebCore/platform/network/soup/CookieJarSoup.cpp:37
#1  0x00007ffff46efede in WebCore::cookiesEnabled (context=0x0)
    at ../../Source/WebCore/platform/network/soup/CookieJarSoup.cpp:141
#2  0x00007ffff44aabc6 in WebCore::cookiesEnabled (document=0x27c61f0)
    at ../../Source/WebCore/loader/CookieJar.cpp:64
#3  0x00007ffff45b5c1f in WebCore::Navigator::cookieEnabled (this=0x28c24b0)
    at ../../Source/WebCore/page/Navigator.cpp:121
#4  0x00007ffff4e6121e in WebCore::jsNavigatorCookieEnabled (exec=0x7fff9eb87528, 
    slotBase=...) at DerivedSources/WebCore/JSNavigator.cpp:295
#5  0x00007ffff3d70e53 in JSC::PropertySlot::getValue (this=0x7fffffffbfc0, 
    exec=0x7fff9eb87528, propertyName=...)
    at ../../Source/JavaScriptCore/runtime/PropertySlot.h:76
#6  0x00007ffff3dbb9e5 in JSC::JSValue::get (this=0x7fffffffc010, 
    exec=0x7fff9eb87528, propertyName=..., slot=...)
    at ../../Source/JavaScriptCore/runtime/JSObject.h:1465
#7  0x00007ffff533ce8d in JSC::LLInt::llint_slow_path_get_by_id (
    exec=0x7fff9eb87528, pc=0x3959678)
    at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:917
#8  0x00007ffff534675c in llint_op_get_by_id ()
   from /home/shivamidow/Projects/webkit-clutter/WebKitBuild/Debug/.libs/libwebkit_clutter-0.so.0
#9  0x00007fff9eb87058 in ?? ()
#10 0x000000000148f1f0 in ?? ()
#11 0x00007fffffffc100 in ?? ()
#12 0x00007ffff52f2eb1 in JSC::JSStack::installTrapsAfterFrame (this=0x0, 
    frame=0x0) at ../../Source/JavaScriptCore/interpreter/JSStackInlines.h:213
#13 0x00007ffff52f1c74 in JSC::JITCode::execute (this=0x7fff940440c0, 
    stack=0x148f1f0, callFrame=0x7fff9eb87058, globalData=0x1487620)
    at ../../Source/JavaScriptCore/jit/JITCode.h:134
#14 0x00007ffff52ef25e in JSC::Interpreter::execute (this=0x148f1e0, 
    program=0x7fff940440a0, callFrame=0x7fffa402f388, thisObj=0x7ffff7e2ffc0)
    at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:979
#15 0x00007ffff53c6bf0 in JSC::evaluate (exec=0x7fffa402f388, source=..., 
    thisValue=..., returnedException=0x7fffffffd780)
    at ../../Source/JavaScriptCore/runtime/Completion.cpp:75
#16 0x00007ffff3db10ed in WebCore::JSMainThreadExecState::evaluate (
    exec=0x7fffa402f388, source=..., thisValue=..., exception=0x7fffffffd780)
    at ../../Source/WebCore/bindings/js/JSMainThreadExecState.h:77
#17 0x00007ffff3de4460 in WebCore::ScriptController::evaluateInWorld (
    this=0x14544f0, sourceCode=..., world=0x14a0f00)
    at ../../Source/WebCore/bindings/js/ScriptController.cpp:141
#18 0x00007ffff3de4562 in WebCore::ScriptController::evaluate (this=0x14544f0, 
    sourceCode=...) at ../../Source/WebCore/bindings/js/ScriptController.cpp:158
---Type <return> to continue, or q <return> to quit---k
#19 0x00007ffff40f64eb in WebCore::ScriptElement::executeScript (this=0x2f48100, 
    sourceCode=...) at ../../Source/WebCore/dom/ScriptElement.cpp:303
q
#20 0x00007ffff42f9262 in WebCore::HTMLScriptRunner::executePendingScriptAndDispatchEvent (this=0x14a10e0, pendingScript=...)
    at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:139
#21 0x00007ffff42f90b5 in WebCore::HTMLScriptRunner::executeParsingBlockingScript (
    this=0x14a10e0) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:118
#22 0x00007ffff42f95e0 in WebCore::HTMLScriptRunner::executeParsingBlockingScripts
    (this=0x14a10e0) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:190
^C/build/buildd/gdb-7.4-2012.04/gdb/frame.c:2374: internal-error: frame_cleanup_after_sniffer: Assertion `frame->prologue_cache == NULL' failed.
Comment 5 ChangSeok Oh 2012-11-15 10:00:18 PST
The context could be null, when document or frame or loader is null and also FrameLoaderClient can't provide a proper networking context. Please see netwrokingContext at CookieJar.cpp
Plus, We've also assumed it could be null elsewhere, so checked null like this.

>static String cookiesForContext(NetworkingContext* context, const KURL& url, bool forHTTPHeader)
>{
>    SoupCookieJar* jar = context ? cookieJarForContext(context) : soupCookieJar(); 
> ...
Comment 6 Alexey Proskuryakov 2012-11-15 10:06:40 PST
Comment on attachment 174455 [details]
Patch

> The context could be null, when document or frame or loader is null

This can never happen in Navigator::cookieEnabled() - it checks that m_frame is not null, and others can never be null.

> and also FrameLoaderClient can't provide a proper networking context.

This looks like the root cause for your port. The client should provide a networking context, it makes no sense for a FrameLoader to not have one.
Comment 7 Alexey Proskuryakov 2012-11-15 10:10:02 PST
Comment on attachment 174455 [details]
Patch

Actually, let me take this back. Even though the real reason for the crash is an incorrect client implementation, it is OK to have a null check here.
Comment 8 Alexey Proskuryakov 2012-11-15 10:10:40 PST
Comment on attachment 174455 [details]
Patch

cq- to discuss Martin's comment.
Comment 9 Martin Robinson 2012-11-15 10:16:02 PST
Can you reproduce this consistently and with what port (GTK+ or EFL)? If you can create a small test case, that'd be great. We also need to merge this into the stable branch if it's an issue with GTK+.