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.
<rdar://problem/26344251>
Created attachment 280188 [details] Notify the client immediately when the network session doesn't exist for a sync XHR request.
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?
(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.
(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?
(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!
Created attachment 280250 [details] Signal the error for all loads.
Should the callback be made asynchronously, with RunLoop::dispatch or equivalent?
(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.
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.
Created attachment 280269 [details] V3: notify the client asynchronously.
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.
(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!
Created attachment 280307 [details] Address review comments before landing.
Comment on attachment 280307 [details] Address review comments before landing. Clearing flags on attachment: 280307 Committed r201593: <http://trac.webkit.org/changeset/201593>
All reviewed patches have been landed. Closing bug.