Bug 100310 – [WK2] Fix assertion after r132386.

• Status: NEW
• Component: WebKit EFL

[Byungwoo Lee]

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&)

[Byungwoo Lee]

Created attachment 170529 [details]
Patch

[Viatcheslav Ostapenko]

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?

[Alexey Proskuryakov]

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.

[Byungwoo Lee]

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?

[Byungwoo Lee]

Andersca told to check this issue. (in irc channel)

[Kangil Han]

Do we need work-around patch to keep EFL bot green for API test at a moment?

[Byungwoo Lee]

(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

[Byungwoo Lee]

(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

[Viatcheslav Ostapenko]

(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?

[Byungwoo Lee]

(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.