Bug 100310 - [WK2] Fix assertion after r132386.
Summary: [WK2] Fix assertion after r132386.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Byungwoo Lee
URL:
Keywords:
Depends on: 100334 100342 102200 102651
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-24 17:09 PDT by Byungwoo Lee
Modified: 2012-11-19 00:06 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.92 KB, patch)
2012-10-24 18:11 PDT, Byungwoo Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Byungwoo Lee 2012-10-24 17:09:04 PDT
Unit test is failed with below assersion after r132386

ASSERTION FAILED: !decoder.destinationID()
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/Source/WebKit2/Platform/CoreIPC/MessageReceiverMap.cpp(79) : bool CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
Comment 1 Byungwoo Lee 2012-10-24 18:11:45 PDT
Created attachment 170529 [details]
Patch
Comment 2 Viatcheslav Ostapenko 2012-10-24 19:14:39 PDT
Comment on attachment 170529 [details]
Patch

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

> Source/WebKit2/Platform/CoreIPC/MessageReceiverMap.cpp:81
> +        // ASSERT(!decoder.destinationID());

Can you do additional condition to assert for the messages that should have destinationID?
Comment 3 Alexey Proskuryakov 2012-10-24 19:23:17 PDT
If the assertion is incorrect, it should be removed, not commented out. I'm not sure if I understand how you determined that it was incorrect though.
Comment 4 Byungwoo Lee 2012-10-24 20:47:54 PDT
Thanks for your review, Viatcheslav and Alexey.

In the r132386, new addMessageReceiver() function (that has destinationID as it's parameter) is added to set messageReceiver to m_messageReceivers. (not m_globalMessageReceivers)

I think the purpose of this patch is that,
if messageReceiver need destinationID, not to use the messageReceiver globally.

So, If there can be two destinationID for DownloadProxy::DidStart, messageReceiver of DownloadProxy should be set for each destinationID.

I thought that this patch is on progress, because some related source doesn't handled. (e.g. messageReceiver for DownloadProxy is set to global message receiver in the WebContext, but DownloadProxy ipc messages uses downloadID for the destinationID.

Is it better to fix each issues on setting messageReceivers of DownloadProxy and VibrationProvider instead of patching temporarily?
Comment 5 Byungwoo Lee 2012-10-24 22:19:10 PDT
Andersca told to check this issue. (in irc channel)
Comment 6 Kangil Han 2012-10-24 22:37:47 PDT
Do we need work-around patch to keep EFL bot green for API test at a moment?
Comment 7 Byungwoo Lee 2012-10-24 23:19:43 PDT
(In reply to comment #6)
> Do we need work-around patch to keep EFL bot green for API test at a moment?

Now, I'm not sure that work-around patch is the right way, because this patch is already work-around :)

I made a patch to fix the assertions from the messageReceiver of WebVibrationProxy : Bug 100334
Comment 8 Byungwoo Lee 2012-10-25 00:28:30 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Do we need work-around patch to keep EFL bot green for API test at a moment?
> 
> Now, I'm not sure that work-around patch is the right way, because this patch is already work-around :)
> 
> I made a patch to fix the assertions from the messageReceiver of WebVibrationProxy : Bug 100334

I made a patch about the assertion from the message receiver of DownloadProxy : Bug 100342
Comment 9 Viatcheslav Ostapenko 2012-10-25 17:52:56 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Do we need work-around patch to keep EFL bot green for API test at a moment?
> > 
> > Now, I'm not sure that work-around patch is the right way, because this patch is already work-around :)
> > 
> > I made a patch to fix the assertions from the messageReceiver of WebVibrationProxy : Bug 100334
> 
> I made a patch about the assertion from the message receiver of DownloadProxy : Bug 100342

So, as I understand, this bug will be fixed by pending commit in bug 100342 and can be closed?
Comment 10 Byungwoo Lee 2012-10-27 05:19:18 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > Do we need work-around patch to keep EFL bot green for API test at a moment?
> > > 
> > > Now, I'm not sure that work-around patch is the right way, because this patch is already work-around :)
> > > 
> > > I made a patch to fix the assertions from the messageReceiver of WebVibrationProxy : Bug 100334
> > 
> > I made a patch about the assertion from the message receiver of DownloadProxy : Bug 100342
> 
> So, as I understand, this bug will be fixed by pending commit in bug 100342 and can be closed?

Yes, but Bug 100334(or Bug 100473) also need to be fixed to close this issue.