Bug 173261 – Retry network process connection if we fail to re-establish it

• Component: Page Loading
• Status: NEW

[Antti Koivisto]

WebProcess::networkConnection() sometimes fails and hits CRASH. It is not clear why it fails but the condition might be transient. It may make sense to retry a few times.

[Antti Koivisto]

<rdar://problem/30359835>

[Antti Koivisto]

Created attachment 312657 [details]
patch

[Brady Eidson]

Comment on attachment 312657 [details]
patch

This kind of change worries me.

Adding this somewhat arbitrary "wait 1/4 second, retry 4 times" introduces a very mysterious behavior to consider and maintain going forward.

Re-establishing the network process connection should succeed the first time.
If it doesn't, that's the bug we should fix.

You mention the problem "might be transient"? Have you shown this patch to actually produce results in practice?

If so, I think we should be much more interested in why it fails the first time.

[Antti Koivisto]

> Adding this somewhat arbitrary "wait 1/4 second, retry 4 times" introduces a
> very mysterious behavior to consider and maintain going forward.

I'm not sure what there is to maintain here.

> Re-establishing the network process connection should succeed the first time.
> If it doesn't, that's the bug we should fix.

Any ideas?

> You mention the problem "might be transient"? Have you shown this patch to
> actually produce results in practice?

No. The best way to establish that is to land it and see if it helps with crash rates.

[Alexey Proskuryakov]

Why only the network process? If we think that process launching is flaky, we should apply the retry to other processes too. 

Have we done anything at all to investigate the root cause yet?

[Antti Koivisto]

(In reply to Alexey Proskuryakov from comment #5)
> Why only the network process? If we think that process launching is flaky,
> we should apply the retry to other processes too. 

Do we have evidence of similar problems with other processes? 

> Have we done anything at all to investigate the root cause yet?

Yes. Looks like the history of this crash goes back to at least 2014 (including analysis by you) and there have been attempts to fix it.

This patch is purely about trying to mitigate.

[Brady Eidson]

(In reply to Antti Koivisto from comment #4)
> > Adding this somewhat arbitrary "wait 1/4 second, retry 4 times" introduces a
> > very mysterious behavior to consider and maintain going forward.
> 
> I'm not sure what there is to maintain here.

The fuzzy behavior of "ah screw it, we'll just keep on trying" as opposed to maintaining software that works the first time.

> > Re-establishing the network process connection should succeed the first time.
> > If it doesn't, that's the bug we should fix.
> 
> Any ideas?

Any ideas about what...? You filed this bug with the statement:
"WebProcess::networkConnection() sometimes fails and hits CRASH. It is not clear why it fails but the condition might be transient. It may make sense to retry a few times."

That is literally the only information I'm going off of here.

It stands to reason that WebProcess::networkConnection() is failing for a reason, yet I see no description of an exploration of that reason.

> > You mention the problem "might be transient"? Have you shown this patch to
> > actually produce results in practice?
> 
> No. The best way to establish that is to land it and see if it helps with
> crash rates.

If the primary goal was to reduce this WebProcess crash, I would agree with this path of exploration.

But I think the primary goal is actually to find out why establishing the network process connection is failing, and this patch does nothing to explore that.