Bug 200126 – Avoid UIProcess hangs due to the WebContent process being stuck on a sync XHR

• Component: WebKit2
• Status: RESOLVED DUPLICATE

[Chris Dumez]

Avoid UIProcess hangs due to the WebContent process being stuck on a sync XHR.

[Chris Dumez]

<rdar://problem/52698157>

[Chris Dumez]

Created attachment 374891 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 374891 [details]
Patch

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

> Source/WebKit/ChangeLog:14
> +        to process touch event-related sync IPC from the UIProcess, which will avoid
> +        UIProcess hangs.

Doesn't that risk re-entering JavaScript and WebCore code?

[Chris Dumez]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 374891 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374891&action=review
> 
> > Source/WebKit/ChangeLog:14
> > +        to process touch event-related sync IPC from the UIProcess, which will avoid
> > +        UIProcess hangs.
> 
> Doesn't that risk re-entering JavaScript and WebCore code?

Well yes, this is the trade-off here. That said, such re-entrancy was already possible before I changed our behavior in Oct 2018.

[Brady Eidson]

Comment on attachment 374891 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:648
> +        // This call may re-enter WebCore.

Why is it okay to reintroduce this?

I made comments in the radar that seem to be ignored in this patch.

[Brady Eidson]

Comment on attachment 374891 [details]
Patch

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

>> Source/WebCore/xml/XMLHttpRequest.cpp:648
>> +        // This call may re-enter WebCore.
> 
> Why is it okay to reintroduce this?
> 
> I made comments in the radar that seem to be ignored in this patch.

Maybe that's too broad of a statement, but this is a huge hammer that says "always respond to all messages" whereas I was envisioning "look at the specific messages that hang and target ways to mitigate just them."

[Chris Dumez]

(In reply to Brady Eidson from comment #5)
> Comment on attachment 374891 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374891&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:648
> > +        // This call may re-enter WebCore.
> 
> Why is it okay to reintroduce this?

Well, the belief is that it is OK as long as we're in the middle of running script and we make sure not to make assumptions about our state right after returning from the sync IPC.

In any case, this is the change that was requested from me to address the hangs in the short-term. I'll let Geoff comment more on this if necessary.

> 
> I made comments in the radar that seem to be ignored in this patch.

The patch does prevent UI-Process hangs, which seems to be what your comment was about? If somebody has a better concrete idea, I am happy to consider but we do not have a lot of time to address those hangs.

[Chris Dumez]

(In reply to Brady Eidson from comment #6)
> Comment on attachment 374891 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374891&action=review
> 
> >> Source/WebCore/xml/XMLHttpRequest.cpp:648
> >> +        // This call may re-enter WebCore.
> > 
> > Why is it okay to reintroduce this?
> > 
> > I made comments in the radar that seem to be ignored in this patch.
> 
> Maybe that's too broad of a statement, but this is a huge hammer that says
> "always respond to all messages" whereas I was envisioning "look at the
> specific messages that hang and target ways to mitigate just them."

I only process incoming *synchronous* messages, aren't those by definition the ones that hang? Or do you want to target specifically the couple of touch-event related ones? It is in theory feasible but in practice it won't look nice to check for specific message names. Also, why only those messages? Those are not any less risky, they end up running script and thus can do anything to WebCore.

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> (In reply to Brady Eidson from comment #6)
> > Comment on attachment 374891 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=374891&action=review
> > 
> > >> Source/WebCore/xml/XMLHttpRequest.cpp:648
> > >> +        // This call may re-enter WebCore.
> > > 
> > > Why is it okay to reintroduce this?
> > > 
> > > I made comments in the radar that seem to be ignored in this patch.
> > 
> > Maybe that's too broad of a statement, but this is a huge hammer that says
> > "always respond to all messages" whereas I was envisioning "look at the
> > specific messages that hang and target ways to mitigate just them."
> 
> I only process incoming *synchronous* messages, aren't those by definition
> the ones that hang? Or do you want to target specifically the couple of
> touch-event related ones? It is in theory feasible but in practice it won't
> look nice to check for specific message names. Also, why only those
> messages? Those are not any less risky, they end up running script and thus
> can do anything to WebCore.

I guess one viable alternative I can think of is add a IPC::SendSyncOption::CancelIfDestinationIsSendingSyncIPC flag on the 2 touch-related IPCs from the UIProcess, and somehow manage to cancel them on the WebContent process side (without running any script or changing any state), if the WebContent process is currently sending a sync IPC. I am not sure yet how easy this would be to implement, but this would potentially be safer. I am unsure how bad it would be for those 2 touch-related IPCs to get cancelled whenever the WebContent process is busy. This might lead to missed touches for examples.

[Brady Eidson]

(In reply to Chris Dumez from comment #9)
> (In reply to Chris Dumez from comment #8)
> > (In reply to Brady Eidson from comment #6)
> > > Comment on attachment 374891 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=374891&action=review
> > > 
> > > >> Source/WebCore/xml/XMLHttpRequest.cpp:648
> > > >> +        // This call may re-enter WebCore.
> > > > 
> > > > Why is it okay to reintroduce this?
> > > > 
> > > > I made comments in the radar that seem to be ignored in this patch.
> > > 
> > > Maybe that's too broad of a statement, but this is a huge hammer that says
> > > "always respond to all messages" whereas I was envisioning "look at the
> > > specific messages that hang and target ways to mitigate just them."
> > 
> > I only process incoming *synchronous* messages, aren't those by definition
> > the ones that hang? 

But *all* incoming sync messages hang
> > Or do you want to target specifically the couple of
> > touch-event related ones? It is in theory feasible but in practice it won't
> > look nice to check for specific message names. Also, why only those
> > messages? Those are not any less risky, they end up running script and thus
> > can do anything to WebCore.

There's two sides where you can tackle this: 
1 - Have the WebProcess side respond to these messages
2 - Have the UIProcess side *not send them*

A naive proposal (which is admittedly racey) is for the WebProcess to tell the ui process "I'm about to do a sync XHR" and then to notify "I'm done with the sync XHR"

The UI process could avoid sending these sync messages in the meantime.
 
> I guess one viable alternative I can think of is add a
> IPC::SendSyncOption::CancelIfDestinationIsSendingSyncIPC flag on the 2
> touch-related IPCs from the UIProcess, and somehow manage to cancel them on
> the WebContent process side (without running any script or changing any
> state), if the WebContent process is currently sending a sync IPC. I am not
> sure yet how easy this would be to implement, but this would potentially be
> safer. I am unsure how bad it would be for those 2 touch-related IPCs to get
> cancelled whenever the WebContent process is busy. This might lead to missed
> touches for examples.

Why is is valuable to make sure we handle ALL touch events during a long running sync XHR?

Wouldn't it be broken to handle dozens/hundreds of touch event messages delayed after the sync XHR finally finishes?

[Chris Dumez]

Will upload alternative patch based on Wenson's proposal at Bug 200138.

[Chris Dumez]

*** This bug has been marked as a duplicate of bug 200138 ***