Bug 131162 – [EFL][WK2] Crash when "Download Linked File" from MiniBrowser context menu is clicked

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Maciej Florek]

Deterministic crash when "Download Linked File" from MiniBrowser context menu is clicked. Crash due to null EwkDownloadJob in WebKit::DownloadManagerEfl::didReceiveResponse.

ASSERTION FAILED: download
webkit.org/Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp(66) : static void WebKit::DownloadManagerEfl::didReceiveResponse(WKContextRef, WKDownloadRef, WKURLResponseRef, const void*)
1   0x7ffff47ea978 WTFCrash
2   0x7ffff7609cec WebKit::DownloadManagerEfl::didReceiveResponse(OpaqueWKContext const*, OpaqueWKDownload const*, OpaqueWKURLResponse const*, void const*)
3   0x7ffff7439fe3
4   0x7ffff7452263 WebKit::DownloadProxy::didReceiveResponse(WebCore::ResourceResponse const&)
5   0x7ffff766df2e void IPC::callMemberFunctionImpl<WebKit::DownloadProxy, void (WebKit::DownloadProxy::*)(WebCore::ResourceResponse const&), std::tuple<WebCore::ResourceResponse>, 0ul>(WebKit::DownloadProxy*, void (WebKit::DownloadProxy::*)(WebCore::ResourceResponse const&), std::tuple<WebCore::ResourceResponse>&&, std::index_sequence<0ul>)
6   0x7ffff766da02 void IPC::callMemberFunction<WebKit::DownloadProxy, void (WebKit::DownloadProxy::*)(WebCore::ResourceResponse const&), std::tuple<WebCore::ResourceResponse>, std::make_index_sequence<1ul> >(std::tuple<WebCore::ResourceResponse>&&, WebKit::DownloadProxy*, void (WebKit::DownloadProxy::*)(WebCore::ResourceResponse const&))
7   0x7ffff766d2fe void IPC::handleMessage<Messages::DownloadProxy::DidReceiveResponse, WebKit::DownloadProxy, void (WebKit::DownloadProxy::*)(WebCore::ResourceResponse const&)>(IPC::MessageDecoder&, WebKit::DownloadProxy*, void (WebKit::DownloadProxy::*)(WebCore::ResourceResponse const&))
8   0x7ffff766cbe5 WebKit::DownloadProxy::didReceiveMessage(IPC::Connection*, IPC::MessageDecoder&)
9   0x7ffff72eb6dc IPC::MessageReceiverMap::dispatchMessage(IPC::Connection*, IPC::MessageDecoder&)
10  0x7ffff7305efb WebKit::ChildProcessProxy::dispatchMessage(IPC::Connection*, IPC::MessageDecoder&)
11  0x7ffff742b2d5 WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection*, IPC::MessageDecoder&)
12  0x7ffff72d9f2a IPC::Connection::dispatchMessage(IPC::MessageDecoder&)
13  0x7ffff72d9ff6 IPC::Connection::dispatchMessage(std::unique_ptr<IPC::MessageDecoder, std::default_delete<IPC::MessageDecoder> >)
14  0x7ffff72da1b7 IPC::Connection::dispatchOneMessage()
15  0x7ffff72eaa7d WTF::FunctionWrapper<void (IPC::Connection::*)()>::operator()(IPC::Connection*)
16  0x7ffff72ea7f0 WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (IPC::Connection::*)()>, void (IPC::Connection*)>::operator()()
17  0x7ffff72c95ef WTF::Function<void ()>::operator()() const
18  0x7ffff72c7fa5 std::_Function_handler<void (), WTF::Function<void ()> >::_M_invoke(std::_Any_data const&)
19  0x7ffff72bcc7e std::function<void ()>::operator()() const
20  0x7ffff76965b9 WTF::RunLoop::performWork()
21  0x7ffff76977ce WTF::RunLoop::wakeUpEvent(void*, void*, unsigned int)
22  0x7ffff64ff497
23  0x7ffff64fe571
24  0x7ffff64fe9b7 ecore_main_loop_begin
25  0x40b3b2 elm_main
26  0x40b3fc main
27  0x7ffff51fa76d __libc_start_main
28  0x405889

[Lukasz Bialek]

Created attachment 246314 [details]
Patch

[Grzegorz Czajkowski]

Comment on attachment 246314 [details]
Patch

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

I like the idea of detaching DownloadManagerEFL from EwkView! Generally looks promising, added few minor comments. I'd like to listen Gyuyoung, Ryuan comments on that.
This patch lacks of API tests but I believe they could be done in a separate patch.

> Source/WebKit2/UIProcess/API/efl/ewk_download_job.h:155
> + * will be called on the Ewk_Download_Job.

Ewk_Download_Job ? I think you meant "Ewk_Context"

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:138
> +    clientCallbacks.m_requested = nullptr;
> +    clientCallbacks.m_failed = nullptr;
> +    clientCallbacks.m_cancelled = nullptr;
> +    clientCallbacks.m_finished = nullptr;
> +    clientCallbacks.m_userData = nullptr;

Please define ClientDownloadCallbacks' constructor or use memset(&clientCallbacks, 0, sizeof(ClientDownloadCallbacks)) for that.

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.h:37
> +struct ClientDownloadCallbacks {

Since we don't expose those callbacks outside DownloadManagerEfl.h/cpp I'd prefer to move the declaration to the cpp file and use forward declaration (struct ClientDownloadsCallbacks;)

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.h:51
> +    void setCallbacks(Ewk_Download_Requested_Cb, Ewk_Download_Failed_Cb, Ewk_Download_Cancelled_Cb,  Ewk_Download_Finished_Cb, void* userData);

setClientCallbacks() ?

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.h:69
> +    ClientDownloadCallbacks clientCallbacks;

m_clientCallbacks as it's class member.

> Tools/ChangeLog:13
> +        Adapt download caalbcks to new callback mechanism

typo: caalbcks -> callbacks

> Tools/MiniBrowser/efl/main.c:850
> +on_download_failed(Ewk_Download_Job_Error *downloadError, void *user_data)

MiniBrowser follows C coding style (downloadError -> download_error).

> Tools/MiniBrowser/efl/main.c:852
> +    const Ewk_Error *error = downloadError->error;

I don't think "const" modifier brings benefits here.

> Tools/MiniBrowser/efl/main.c:2302
> +    // Set callbacks for download events.
> +    ewk_context_download_callbacks_set(context, on_download_request, on_download_failed, 0, on_download_finished, window);

I'd rather move them to context related code in elm_main(), there we already have

    ewk_context_process_model_set(context, EWK_PROCESS_MODEL_MULTIPLE_SECONDARY);
    ewk_context_favicon_database_directory_set(context, NULL);

[Ryuan Choi]

Comment on attachment 246314 [details]
Patch

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

Thanks for the patch.
I leave some comments.

> Source/WebKit2/ChangeLog:15
> +        Propose a new callbacks which are view independent and detach EFL's DownloadMaager
> +        from EwkView. It makes DownloadManagerEFL consistent with WKContextDownload.
> +        Fix the crash and restore download functionality.

I am not sure.
Doesn't we have any use cases which need to distinguish which ewk_view(tab) requests this download?

> Source/WebKit2/ChangeLog:32
> +        Old tests do not match new feature. Separate bug will be introduced.

Bug ID?

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:374
> +EAPI void ewk_context_download_callbacks_set(Ewk_Context* context,
> +    Ewk_Download_Requested_Cb download_requested_func,
> +    Ewk_Download_Failed_Cb download_failed_func,
> +    Ewk_Download_Cancelled_Cb download_cancelled_func,
> +    Ewk_Download_Finished_Cb download_finished_func,
> +    void* data);

EFL looks prefer the spaces to align first letter of first parameter with others if we define API using multiple line.

For example, in Ecore_File.h
EAPI Eina_Bool ecore_file_download(const char *url,
                                   const char *dst,
                                   ...);

And style of * is wrong for the first parameter.

> Source/WebKit2/UIProcess/API/efl/ewk_download_job_private.h:74
> +    EwkDownloadJob(WKDownloadRef download);

Let's add "explicit"

>> Tools/MiniBrowser/efl/main.c:852
>> +    const Ewk_Error *error = downloadError->error;
> 
> I don't think "const" modifier brings benefits here.

Instead, how about define `error` field of Ewk_Download_Job_Error with "const" modifier?

>> Tools/MiniBrowser/efl/main.c:2302
>> +    ewk_context_download_callbacks_set(context, on_download_request, on_download_failed, 0, on_download_finished, window);
> 
> I'd rather move them to context related code in elm_main(), there we already have
> 
>     ewk_context_process_model_set(context, EWK_PROCESS_MODEL_MULTIPLE_SECONDARY);
>     ewk_context_favicon_database_directory_set(context, NULL);

+1

[Csaba Osztrogonác]

Comment on attachment 246314 [details]
Patch

r- now based on previous comments, it isn't ready for landing yet.

[Lukasz Bialek]

Created attachment 247847 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:370:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:371:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:372:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 11 files


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

[Lukasz Bialek]

Hello,

I have uploaded new patch with (almost) all comments included. To answer your questions:

> I am not sure.
> Doesn't we have any use cases which need to distinguish which ewk_view(tab)
> requests this download?

As far as I know, all modern browsers have general download manager common for all tabs/windows. When user starts to download a file, dialog bog appears and then download is conducted by an agent located next to address bar. When it is finished, agent's icon is blinking, not a tab.

> Bug ID?

I will introduce new bug, when approach from this bug will be accepted. Now there is no point for this.

> ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

This is caused by space aligment in ewk_context.h (that is why I used only 4 spaces there in initial patch). As far as I know checking style in *.h files should be disabled so probably those messages can be ignored. 

Best regards,
Lukasz Bialek

[Ryuan Choi]

(In reply to comment #7)
> Hello,
> 
> I have uploaded new patch with (almost) all comments included. To answer
> your questions:
> 
> > I am not sure.
> > Doesn't we have any use cases which need to distinguish which ewk_view(tab)
> > requests this download?
> 
> As far as I know, all modern browsers have general download manager common
> for all tabs/windows. When user starts to download a file, dialog bog
> appears and then download is conducted by an agent located next to address
> bar. When it is finished, agent's icon is blinking, not a tab.
> 
OK.

> > Bug ID?
> 
> I will introduce new bug, when approach from this bug will be accepted. Now
> there is no point for this.
> 
> > ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> This is caused by space aligment in ewk_context.h (that is why I used only 4
> spaces there in initial patch). As far as I know checking style in *.h files
> should be disabled so probably those messages can be ignored. 
> 

Hmm, we can consider to add exception for check-webkit-style.
But I am not sure which one is better for us.

gyuyoung, what do you think about it?

> Best regards,
> Lukasz Bialek

[Gyuyoung Kim]

Comment on attachment 247847 [details]
Patch

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

> Source/WebKit2/ChangeLog:13
> +        Propose a new callbacks which are view independent and detach EFL's DownloadMaager

Typo: DownloadMaager -> DownloadManager

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_download_job.cpp:-44
> -class EWK2DownloadJobTest : public EWK2UnitTestBase {

Why should we remove this unit test ? I think we need to modify this unit test based on this change.

> Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:164
> +    RefPtr<EwkDownloadJob> ewkDownload = EwkDownloadJob::create(download);

I think EwkDownloadJob::create() is not necessary. We can make an instance with new EwkDownloadJob if we move its constructor from private to public.

[Gyuyoung Kim]

(In reply to comment #8)
> (In reply to comment #7)
> > Hello,
> > 
> > I have uploaded new patch with (almost) all comments included. To answer
> > your questions:
> > 
> > > I am not sure.
> > > Doesn't we have any use cases which need to distinguish which ewk_view(tab)
> > > requests this download?
> > 
> > As far as I know, all modern browsers have general download manager common
> > for all tabs/windows. When user starts to download a file, dialog bog
> > appears and then download is conducted by an agent located next to address
> > bar. When it is finished, agent's icon is blinking, not a tab.
> > 
> OK.
> 
> > > Bug ID?
> > 
> > I will introduce new bug, when approach from this bug will be accepted. Now
> > there is no point for this.
> > 
> > > ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> > 
> > This is caused by space aligment in ewk_context.h (that is why I used only 4
> > spaces there in initial patch). As far as I know checking style in *.h files
> > should be disabled so probably those messages can be ignored. 
> > 
> 
> Hmm, we can consider to add exception for check-webkit-style.
> But I am not sure which one is better for us.
> 
> gyuyoung, what do you think about it?

I think we can land this patch without check-webkit-style change.

[Lukasz Bialek]

Created attachment 247943 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:370:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:371:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:372:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 13 files


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

[Lukasz Bialek]

Hello,

New patch uploaded. 

> Typo: DownloadMaager -> DownloadManager

Fixed.

> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_download_job.cpp:-44
> > -class EWK2DownloadJobTest : public EWK2UnitTestBase {
> 
> Why should we remove this unit test ? I think we need to modify this unit
> test based on this change.

Tests are modified and enabled. They pass.

> > Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:164
> > +    RefPtr<EwkDownloadJob> ewkDownload = EwkDownloadJob::create(download);
> 
> I think EwkDownloadJob::create() is not necessary. We can make an instance
> with new EwkDownloadJob if we move its constructor from private to public.

Unfortunately, when I tried to convert it to unique_ptr approach, I encountered a problem with HashMap add operation ("use of deleted function 'std::unique_ptr<_Tp, _Dp>& std::unique_ptr<_Tp, _Dp>::operator=(...)". I would leave it for now because it will need some further investigation about HashMap and this problem is not connected with subject of this patch. It can be easily fixed in separete bug.

Best regards,
Lukasz Bialek

[Gyuyoung Kim]

Comment on attachment 247943 [details]
Patch

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

LGTM except for my small comment.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_download_job.cpp:123
> +    static void on_download_cancelled(EwkObject* eventInfo, void* userData)

eventInfo is not used inside this function. It can cause unused param build warning.

[Lukasz Bialek]

Created attachment 247946 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:370:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:371:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:372:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 13 files


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

[WebKit Commit Bot]

Comment on attachment 247946 [details]
Patch

Clearing flags on attachment: 247946

Committed r181076: <http://trac.webkit.org/changeset/181076>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.