Bug 131675 - REGRESSION(r165841): Messages sent before the child process is launched are never sent
Summary: REGRESSION(r165841): Messages sent before the child process is launched are n...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, InRadar, Regression
: 131037 (view as bug list)
Depends on:
Blocks: 130419 131729 132384 133215
  Show dependency treegraph
 
Reported: 2014-04-15 03:45 PDT by Carlos Garcia Campos
Modified: 2014-05-24 00:18 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.74 KB, patch)
2014-04-15 04:51 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (4.94 KB, patch)
2014-05-23 10:26 PDT, Carlos Garcia Campos
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2014-04-15 03:45:34 PDT
Since r165841, the connection is opened after the pending messages are sent, because connectionWillOpen might send messages that we want to happen after the ones already pending. The problem is that Connection::canSendOutgoingMessages() returns false when connection hasn't been opened (m_isConnected). So, I think we should not send any message before Connection::open(). This broke a lot of GTK+ unit tests since it's very common in unit tests to have messages sent before the web process has been launched. I think we can change connectionWillOpen, and use ConnectionDidOpen instead, called after Connection::open.
Comment 1 Carlos Garcia Campos 2014-04-15 04:51:17 PDT
Created attachment 229366 [details]
Patch

I think we should ensure no messages are sent before the connection has been opened. This patch moves the visited link provider process initialization after the connection has been opened.
Comment 2 Alexey Proskuryakov 2014-04-15 10:09:00 PDT
<rdar://problem/16621575>
Comment 3 Ryuan Choi 2014-04-16 10:35:59 PDT
This patch solved many crashes of layout tests/unit tests on the EFL port.
Comment 4 Carlos Garcia Campos 2014-04-23 23:46:27 PDT
Could someone review this, please? Otherwise I would suggest to rollout r165841.
Comment 5 Carlos Alberto Lopez Perez 2014-05-20 04:39:57 PDT
This patch also fixes bug 127352. See: https://bugs.webkit.org/show_bug.cgi?id=127352#c14
Comment 6 Csaba Osztrogonác 2014-05-20 04:58:50 PDT
I think it is a duplicate of Bug131037. Zsolt, could you check if this 
patch solves the problem you ran into with EFL perf bot in Bug131037.

As far as I know r165841 is reverted locally on the EFL perf bot.
Comment 7 Zsolt Borbely 2014-05-21 07:34:35 PDT
I have double-checked, this patch solves the problem that was mentioned in https://bugs.webkit.org/show_bug.cgi?id=131037 Thanks.
Comment 8 Zsolt Borbely 2014-05-21 07:36:25 PDT
*** Bug 131037 has been marked as a duplicate of this bug. ***
Comment 9 Csaba Osztrogonác 2014-05-23 05:59:13 PDT
In my opinion, if more than a month wasn't enough for WK2 owners to review 
the fix for the regression they caused, we should simply revert r165841.
Comment 10 Martin Robinson 2014-05-23 09:50:43 PDT
(In reply to comment #9)
> In my opinion, if more than a month wasn't enough for WK2 owners to review 
> the fix for the regression they caused, we should simply revert r165841.

Perhaps we should just send a polite request to the mailing list asking for a review. It seems like this patch is quite important, as it's causing our bots to rot.
Comment 11 Carlos Alberto Lopez Perez 2014-05-23 10:04:39 PDT
(In reply to comment #4)
> Could someone review this, please? 

The patch not longer applies on trunk. It needs to be rebased.
Comment 12 Carlos Garcia Campos 2014-05-23 10:24:14 PDT
(In reply to comment #11)
> (In reply to comment #4)
> > Could someone review this, please? 
> 
> The patch not longer applies on trunk. It needs to be rebased.

Yes, now we have the same problem with the user content controller.
Comment 13 Carlos Garcia Campos 2014-05-23 10:26:33 PDT
Created attachment 231972 [details]
Rebased patch
Comment 14 Anders Carlsson 2014-05-23 11:28:14 PDT
Comment on attachment 231972 [details]
Rebased patch

Patch looks good, please land it. If it turns out to be a problem for Mac we can roll it out and figure out a better way to solve it!
Comment 15 Carlos Garcia Campos 2014-05-24 00:18:30 PDT
Committed r169306: <http://trac.webkit.org/changeset/169306>