Bug 158239 – Notify client immediately if network session doesn't exist for a synchronous XHR load.

• Component: WebKit2
• Status: RESOLVED FIXED

[Yongjun Zhang]

For a synchronous XHR load, if the network session doesn't exist, we should notify NetworkLoadClient immediately. Otherwise the Web process could hang when waiting for the synchronous load to finish.

[Yongjun Zhang]

<rdar://problem/26344251>

[Yongjun Zhang]

Created attachment 280188 [details]
Notify the client immediately when the network session doesn't exist for a sync XHR request.

[Brady Eidson]

Comment on attachment 280188 [details]
Notify the client immediately when the network session doesn't exist for a sync XHR request.

Why is this only an issue for sync loads? It seems like it's an issue for async loads, too.

Sync loads cause a larger issue because the WebProcess is purposefully hanging for them, but isn't it also bad for the WebProcess to think an async load is happening.... forever...? Even though it will never happen?

[Yongjun Zhang]

(In reply to comment #3)
> Comment on attachment 280188 [details]
> Notify the client immediately when the network session doesn't exist for a
> sync XHR request.
> 
> Why is this only an issue for sync loads? It seems like it's an issue for
> async loads, too.
> 
> Sync loads cause a larger issue because the WebProcess is purposefully
> hanging for them, but isn't it also bad for the WebProcess to think an async
> load is happening.... forever...? Even though it will never happen?

I wasn't able to reproduce with async load. We could hit the point where the network session is removed for an async load. However, since this only happens when we closing a web page, Web Process will cancel the load immediately and we won't have a long waiting WebProcess.

[Brady Eidson]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 280188 [details]
> > Notify the client immediately when the network session doesn't exist for a
> > sync XHR request.
> > 
> > Why is this only an issue for sync loads? It seems like it's an issue for
> > async loads, too.
> > 
> > Sync loads cause a larger issue because the WebProcess is purposefully
> > hanging for them, but isn't it also bad for the WebProcess to think an async
> > load is happening.... forever...? Even though it will never happen?
> 
> I wasn't able to reproduce with async load. We could hit the point where the
> network session is removed for an async load. However, since this only
> happens when we closing a web page, Web Process will cancel the load
> immediately and we won't have a long waiting WebProcess.

That's *currently* the only time a Session can be removed, but I foresee that could change in the future.

I don't see any downside to immediately signaling the error for all loads.

You've looked at the code more recently - Do you foresee any downside?

[Yongjun Zhang]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Comment on attachment 280188 [details]
> > > Notify the client immediately when the network session doesn't exist for a
> > > sync XHR request.
> > > 
> > > Why is this only an issue for sync loads? It seems like it's an issue for
> > > async loads, too.
> > > 
> > > Sync loads cause a larger issue because the WebProcess is purposefully
> > > hanging for them, but isn't it also bad for the WebProcess to think an async
> > > load is happening.... forever...? Even though it will never happen?
> > 
> > I wasn't able to reproduce with async load. We could hit the point where the
> > network session is removed for an async load. However, since this only
> > happens when we closing a web page, Web Process will cancel the load
> > immediately and we won't have a long waiting WebProcess.
> 
> That's *currently* the only time a Session can be removed, but I foresee
> that could change in the future.
> 
> I don't see any downside to immediately signaling the error for all loads.
> 
> You've looked at the code more recently - Do you foresee any downside?

I don't see any downside either, and I agree signaling the error for all loads sounds more future-proof. I will submit another path. Thanks!

[Yongjun Zhang]

Created attachment 280250 [details]
Signal the error for all loads.

[Anders Carlsson]

Should the callback be made asynchronously, with RunLoop::dispatch or equivalent?

[Brady Eidson]

(In reply to comment #8)
> Should the callback be made asynchronously, with RunLoop::dispatch or
> equivalent?

This was going to be my review feedback. It should be asynchronous.

[Brady Eidson]

Comment on attachment 280250 [details]
Signal the error for all loads.

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

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:62
> +        m_client.didFailLoading(internalError(m_currentRequest.url()));

Synchronously calling back to the client - especially in the constructor - is probably inviting trouble.

We should spin the runloop.

[Yongjun Zhang]

Created attachment 280269 [details]
V3: notify the client asynchronously.

[Brady Eidson]

Comment on attachment 280269 [details]
V3: notify the client asynchronously.

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

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:64
> +        RunLoop::main().dispatch([this, url = parameters.request.url()] {
> +            didCompleteWithError(internalError(url));
> +        });

This is a super nit pick, and the first time I've ever suggested it...

But we've been looking at all of our lambda usage lately, and going over things like "RunLoop::dispatch" and "callOnMainThread" with a fine toothed comb. We've found a number of thread safety issues.

At a glance, this looks like a potential thread safety issue, because it's unsafe to send a URL across threads without making an isolatedCopy() of it first.

Of course this isn't *actually* a thread safety issue because it's not cross thread - RunLoop::main() happens to also be the current RunLoop.

If the dispatch was instead "RunLoop::current().dispatch(...)", then I wouldn't even have considered it a potential thread safety issue.

If you'd like to make that change, that'd be nice. I haven't thought about it a lot yet, but I think that's what we should do going forward and will likely be discussing with a larger audience soon.

r+ either way.

[Yongjun Zhang]

(In reply to comment #12)
> Comment on attachment 280269 [details]
> V3: notify the client asynchronously.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280269&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkLoad.cpp:64
> > +        RunLoop::main().dispatch([this, url = parameters.request.url()] {
> > +            didCompleteWithError(internalError(url));
> > +        });
> 
> This is a super nit pick, and the first time I've ever suggested it...
> 
> But we've been looking at all of our lambda usage lately, and going over
> things like "RunLoop::dispatch" and "callOnMainThread" with a fine toothed
> comb. We've found a number of thread safety issues.
> 
> At a glance, this looks like a potential thread safety issue, because it's
> unsafe to send a URL across threads without making an isolatedCopy() of it
> first.

Yes, I had the same concern at first, but I convinced myself it won't be an issue - it looks like we expected NetworkLoad to always work on main thread (e.g., we assert this in couple of places). 
> 
> Of course this isn't *actually* a thread safety issue because it's not cross
> thread - RunLoop::main() happens to also be the current RunLoop.
> 
> If the dispatch was instead "RunLoop::current().dispatch(...)", then I
> wouldn't even have considered it a potential thread safety issue.
> 
> If you'd like to make that change, that'd be nice. I haven't thought about
> it a lot yet, but I think that's what we should do going forward and will
> likely be discussing with a larger audience soon.

Sure thing! I will make this change before landing.
> 
> r+ either way.

Thank you for the review!

[Yongjun Zhang]

Created attachment 280307 [details]
Address review comments before landing.

[WebKit Commit Bot]

Comment on attachment 280307 [details]
Address review comments before landing.

Clearing flags on attachment: 280307

Committed r201593: <http://trac.webkit.org/changeset/201593>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.