Bug 62628 – [EFL] Crash when running EWebLauncher

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Michal Pakula vel Rutka]

Overview:
EWebLaucher crashes on start loading google page. In fact in my case it is redirected to google.pl.

Steps to reproduce:
Run EWebLauncher without any parameters.

The crash happens in WebCore/loader/DocumentWriter.cpp in line:

m_parser->flush(this);
if (!m_parser)
    return;

Moving NULL-check above m_parser->flush(this); fixes this problem.

[Michal Pakula vel Rutka]

a backtrace:

Program received signal SIGSEGV, Segmentation fault.
0xb58c8595 in WebCore::DocumentWriter::endIfNotLoadingMainResource (this=0x8212a28)
    at /home/michal/source/host/autobuilder/profiles/autobuilder/builds/WebKit/WebKit/Source/WebCore/loader/DocumentWriter.cpp:223
223	    m_parser->flush(this);
(gdb) bt
#0  0xb58c8595 in WebCore::DocumentWriter::endIfNotLoadingMainResource (this=0x8212a28)
    at /home/michal/source/host/autobuilder/profiles/autobuilder/builds/WebKit/WebKit/Source/WebCore/loader/DocumentWriter.cpp:223
#1  0xb58c8518 in WebCore::DocumentWriter::end (this=0x8212a28) at /home/michal/source/host/autobuilder/profiles/autobuilder/builds/WebKit/WebKit/Source/WebCore/loader/DocumentWriter.cpp:209
#2  0xb58be053 in WebCore::DocumentLoader::finishedLoading (this=0x8212980) at /home/michal/source/host/autobuilder/profiles/autobuilder/builds/WebKit/WebKit/Source/WebCore/loader/DocumentLoader.cpp:288
#3  0xb58d86b8 in WebCore::FrameLoader::finishedLoading (this=0x820db68) at /home/michal/source/host/autobuilder/profiles/autobuilder/builds/WebKit/WebKit/Source/WebCore/loader/FrameLoader.cpp:2077
#4  0xb58eb9c4 in WebCore::MainResourceLoader::didFinishLoading (this=0x8211348, finishTime=0)
    at /home/michal/source/host/autobuilder/profiles/autobuilder/builds/WebKit/WebKit/Source/WebCore/loader/MainResourceLoader.cpp:485
#5  0xb58fd083 in WebCore::ResourceLoader::didFinishLoading (this=0x8211348, finishTime=0)
    at /home/michal/source/host/autobuilder/profiles/autobuilder/builds/WebKit/WebKit/Source/WebCore/loader/ResourceLoader.cpp:449
#6  0xb5eb0b5c in WebCore::readCallback (source=0x8162520, asyncResult=0xb2817488, data=0x0)
    at /home/michal/source/host/autobuilder/profiles/autobuilder/builds/WebKit/WebKit/Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:792
#7  0xb7cb81e5 in async_ready_callback_wrapper (source_object=0x8162520, res=0xb2817488, user_data=0x0) at ginputstream.c:470
#8  0xb7cca114 in g_simple_async_result_complete (simple=0xb2817488) at gsimpleasyncresult.c:749
#9  0xb7da7b98 in read_async_done (stream=0x8162520) at soup-http-input-stream.c:723
#10 0xb7da70ae in soup_http_input_stream_finished (msg=0x8160d08, stream=0x8162520) at soup-http-input-stream.c:310
#11 0xb7c42e8c in g_cclosure_marshal_VOID__VOID (closure=0x8214418, return_value=0x0, n_param_values=1, param_values=0xb2817218, invocation_hint=0xbfffd830, marshal_data=0xb7da7070) at gmarshal.c:79
#12 0xb7c332ca in g_closure_invoke (closure=0x8214418, return_value=0x0, n_param_values=1, param_values=0xb2817218, invocation_hint=0xbfffd830) at gclosure.c:771
#13 0xb7c4c013 in signal_emit_unlocked_R (node=<value optimized out>, detail=<value optimized out>, instance=0x8160d08, emission_return=0x0, instance_and_params=0xb2817218) at gsignal.c:3256
#14 0xb7c4d4f0 in g_signal_emit_valist (instance=0x8160d08, signal_id=23, detail=0, var_args=0xbfffd9fc "\364\377ܷ\271\220ڷ\364\377ܷh\332\377\277\262\302۷\b\r\026\bL\332\377\277\322>") at gsignal.c:2987
#15 0xb7c4dc02 in g_signal_emit (instance=0x8160d08, signal_id=23, detail=0) at gsignal.c:3044
#16 0xb7da90df in soup_message_finished (msg=0x8160d08) at soup-message.c:1086
#17 0xb7dbc2b2 in process_queue_item (item=0x8162e08, should_prune=<value optimized out>, loop=1) at soup-session-async.c:376
#18 0xb7dbc5f6 in run_queue (sa=<value optimized out>) at soup-session-async.c:418
#19 0xb7dbc6b8 in idle_run_queue (sa=0x807d430) at soup-session-async.c:441
#20 0xb7b3aeb1 in g_idle_dispatch (source=0x8208b38, callback=0, user_data=0x807d430) at gmain.c:4844
#21 0xb7b3dbe5 in g_main_dispatch (context=0x8163868) at gmain.c:2477
#22 g_main_context_dispatch (context=0x8163868) at gmain.c:3050
#23 0xb7e07483 in _ecore_glib_select__locked (ecore_fds=8, rfds=0xbfffdf48, wfds=0xbfffdec8, efds=0xbfffde48, ecore_timeout=0xbfffdfc8) at ecore_glib.c:154
#24 _ecore_glib_select (ecore_fds=8, rfds=0xbfffdf48, wfds=0xbfffdec8, efds=0xbfffde48, ecore_timeout=0xbfffdfc8) at ecore_glib.c:182
#25 0xb7e0240e in _ecore_main_select (timeout=0.042227788828313351) at ecore_main.c:1161
#26 0xb7e02d6f in _ecore_main_loop_iterate_internal (once_only=0) at ecore_main.c:1516
#27 0xb7e02db7 in ecore_main_loop_begin () at ecore_main.c:698
#28 0x0804c688 in main (argc=2, argv=0xbffff1d4) at /home/michal/source/host/autobuilder/profiles/autobuilder/builds/WebKit/WebKit/Tools/EWebLauncher/main.c:906

[Michal Pakula vel Rutka]

Created attachment 97095 [details]
proposed patch

[WebKit Review Bot]

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

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

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


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

[Gyuyoung Kim]

Comment on attachment 97095 [details]
proposed patch

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

> Source/WebCore/ChangeLog:4
> +

Add Bug Title and Address

[Gyuyoung Kim]

LGTM. This crash comes from Bug 62499.

m_parser->flush(this) is called before checking if m_parser is null.

[Gyuyoung Kim]

(In reply to comment #4)
> (From update of attachment 97095 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97095&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +
> 
> Add Bug Title and Address

Please upload new patch with new ChangeLog.

[WebKit Review Bot]

Comment on attachment 97095 [details]
proposed patch

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

New failing tests:
fast/frames/set-parent-src-synchronously.xhtml
fast/parser/iframe-sets-parent-to-javascript-url.html

[WebKit Review Bot]

Created attachment 97096 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Raphael Kubo da Costa (:rakuco)]

We have experienced the same crash here as well. It'd be nice if abarth could comment if the proposed patch makes sense.

[Michal Pakula vel Rutka]

Created attachment 97101 [details]
fixed Changelog

[WebKit Review Bot]

Comment on attachment 97101 [details]
fixed Changelog

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

New failing tests:
fast/frames/set-parent-src-synchronously.xhtml
fast/parser/iframe-sets-parent-to-javascript-url.html

[WebKit Review Bot]

Created attachment 97104 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

Comment on attachment 97101 [details]
fixed Changelog

Makes sense.  How do we test this?

[Eric Seidel (no email)]

@abarth: looks like the cr-linux-ews is just confused.

[Adam Barth]

(In reply to comment #14)
> @abarth: looks like the cr-linux-ews is just confused.

It's not confused.  Those test failures are real.  The patch is wrong.

[Adam Barth]

Comment on attachment 97101 [details]
fixed Changelog

This patch causes crashes.

Can you write a test that demonstrates the issue?  It seems likely that this is a bug in the EFL WebKit Layer.  You're probably calling some sort of end-ish function too many times.

[Lucas De Marchi]

(In reply to comment #16)
> (From update of attachment 97101 [details])
> This patch causes crashes.
> 
> Can you write a test that demonstrates the issue?  It seems likely that this is a bug in the EFL WebKit Layer.  You're probably calling some sort of end-ish function too many times.

(In reply to comment #15)
> (In reply to comment #14)
> > @abarth: looks like the cr-linux-ews is just confused.
> 
> It's not confused.  Those test failures are real.  The patch is wrong.

The patch may be wrong, but this piece of code is wrong as well. m_parser cannot disappear after calling m_parser->flush(this), so why checking if m_parser is 0 after that(In reply to comment #16)
> (From update of attachment 97101 [details])
> This patch causes crashes.
> 
> Can you write a test that demonstrates the issue?  It seems likely that this is a bug in the EFL WebKit Layer.  You're probably calling some sort of end-ish function too many times.

Looking the backtrace Michal provided, I can say for sure it's not specific to EFL. If DocumentWriter::endIfNotLoadingMainResource() cannot be called with m_parser == 0, it seems a problem in soup and therefore this can be reproduced in GTK port too. Michal, could you test this on GTK as well?

Anyway, the patch might not be right but this piece of code does look wrong. It's not possible that m_parser becomes 0 after calling m_parser->flush(this) and the check should be removed or moved up like the patch Michal sent.

[Gyuyoung Kim]

(In reply to comment #17)
 
> Looking the backtrace Michal provided, I can say for sure it's not specific to EFL. If DocumentWriter::endIfNotLoadingMainResource() cannot be called with m_parser == 0, it seems a problem in soup and therefore this can be reproduced in GTK port too. Michal, could you test this on GTK as well?
> 

Hmm, there is no crash on GTK port. (I'm using r88786 based on 2011.06.14.)
As Adam said, it seems we need to check if this crash only occurs on EFL layer.

[Ryuan Choi]

(In reply to comment #18)
> (In reply to comment #17)
> 
> Anyway, the patch might not be right but this piece of code does look wrong. It's not possible that m_parser becomes 0 after calling m_parser->flush(this) and the check should be removed or moved up like the patch Michal sent.

Although it looks wrong, I think that it can be possible if m_parser->flush make recursion.

I tested my assumption like below.
@@ -220,11 +221,14 @@ void DocumentWriter::endIfNotLoadingMainResource()
     RefPtr<Frame> protector(m_frame);
 
     // FIXME: m_parser->finish() should imply m_parser->flush().
+    static int counter = 0;
+    printf (" counter %d entered\n", ++counter);
     m_parser->flush(this);
+    printf (" counter %d leaved\n", counter--);
     if (!m_parser)
         return;
     m_parser->finish();
     m_parser = 0;
 }

# ./WebKitBuild/Release/Programs/EWebLauncher file:///workspace/webkits/efl-webkit/LayoutTests/fast/parser/iframe-sets-parent-to-javascript-url.html
 counter 1 entered
 counter 1 leaved
 counter 1 entered
 counter 2 entered
 counter 2 leaved
 counter 1 leaved
 counter 1 entered
factor=1.000000, intFactor=100, zoomLevels[5]=100, zoomLevels[6]=110
 counter 2 entered
 counter 2 leaved
 counter 1 leaved

Michal,
This issue is because FrameLoaderClientEfl doesn't implement makeRepresentation or revertToProvisionalState.
WebKit/EFL should check it and clear parser in finishedLoading like other ports.

However, I am studying why used setEncoding for it. :)

[Eric Seidel (no email)]

It seems like regardless, that "if" needs a comment (possibly from Adam?) explaining what its for.

[Ryuan Choi]

Created attachment 98089 [details]
Patch

[Ryuan Choi]

(In reply to comment #21)
> Created an attachment (id=98089) [details]
> Patch

I prepared patch instead of Michal after discussed.
All of other ports call setEncoding in FrameLoderClient::finishedLoading() to remove document and clear parser, but WebKit/EFL didn't.

With this patch, this crash will be passed.

BTW, I don't know endIfNotLoadingMainResource should check isLoadingMainResource() which already set false in caller().
Only DocumentWriter::end() call endIfNotLoadingMainResource().

void DocumentWriter::end()
{
    m_frame->loader()->didEndDocument(); // <- set m_isLoadingMainResource to false.
    endIfNotLoadingMainResource();
}

void DocumentWriter::endIfNotLoadingMainResource()
{
    if (m_frame->loader()->isLoadingMainResource() || !m_frame->page() || !m_frame->document())
        return;
...
}

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #22)
> (In reply to comment #21)
> > Created an attachment (id=98089) [details] [details]
> > Patch
> 
> I prepared patch instead of Michal after discussed.

Please mark Michal's patch as obsolete then.

> All of other ports call setEncoding in FrameLoderClient::finishedLoading() to remove document and clear parser, but WebKit/EFL didn't.

Unfortunately the other ports don't say much about why it has been done this way: it was introduced in chromium in revision 50742 which just upstreamed a lot of code, in qt in revision 67268 which just copied chromium's behaviour to fix some tests and in gtk in revision 77596 due to unrelated reasons.

The same patch was part of code we were going to upstream with EFL's DumpRenderTree anyway. If it also fixes this crash, I'm all for it.

[Antonio Gomes]

Comment on attachment 98089 [details]
Patch

Could you rename m_hasRepresentation to something else?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #24)
> (From update of attachment 98089 [details])
> Could you rename m_hasRepresentation to something else?

Not that I disagree, but may I ask why? It's what chromium, gtk and qt use. mac, OTOH, uses representationFinishedLoading.

[Antonio Gomes]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 98089 [details] [details])
> > Could you rename m_hasRepresentation to something else?
> 
> Not that I disagree, but may I ask why? It's what chromium, gtk and qt use. mac, OTOH, uses representationFinishedLoading.

question is: what has a representation? representation of what?

[Ryuan Choi]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (From update of attachment 98089 [details] [details] [details])
> > > Could you rename m_hasRepresentation to something else?
> > 
> > Not that I disagree, but may I ask why? It's what chromium, gtk and qt use. mac, OTOH, uses representationFinishedLoading.
> 
> question is: what has a representation? representation of what?

As my poor reading of mac port,
makeRepresentation() create representation which contains MIMEType and whether plugins allowed.
(WebHTMLRepresentation? I am not sure.)

When finishedLoading is called, representation checks whether it is pluginView, whether it is DisplayingWebArchive and so on. (Below code)

(in WebHTMLRepresentation.mm)
- (void)finishedLoadingWithDataSource:(WebDataSource *)dataSource
{
    WebFrame* webFrame = [dataSource webFrame];

    if (_private->pluginView) {
        [_private->manualLoader pluginViewFinishedLoading:_private->pluginView];
        return;
    }   

    if (!webFrame)
        return;

    if (![self _isDisplayingWebArchive]) {
        // Telling the frame we received some data and passing nil as the data is our
        // way to get work done that is normally done when the first bit of data is
        // received, even for the case of a document with no data (like about:blank).
        [webFrame _commitData:nil];
    }   

    WebView *webView = [webFrame webView];
    if ([webView isEditable])
        core(webFrame)->editor()->applyEditingStyleToBodyElement();
}

WebKit/EFL (and other ports also) doesn't have representaion and just checked similar in FrameLoaderClient::finishedLoading.

If you want, I can change it to m_hasDocumentRepresentation or m_hasHTMLRepresentation or your suggestion.
But now, I'd like to keep current name like other ports except Mac port.

[Antonio Gomes]

Comment on attachment 98089 [details]
Patch

Could you rename m_hasRepresentation to something else?

[WebKit Review Bot]

Comment on attachment 98089 [details]
Patch

Clearing flags on attachment: 98089

Committed r89572: <http://trac.webkit.org/changeset/89572>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.