<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Notify client immediately if network session doesn't exist for a synchronous XHR load."
href="https://bugs.webkit.org/show_bug.cgi?id=158239#c13">Comment # 13</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Notify client immediately if network session doesn't exist for a synchronous XHR load."
href="https://bugs.webkit.org/show_bug.cgi?id=158239">bug 158239</a>
from <span class="vcard"><a class="email" href="mailto:yongjun_zhang@apple.com" title="Yongjun Zhang <yongjun_zhang@apple.com>"> <span class="fn">Yongjun Zhang</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=158239#c12">comment #12</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=280269&action=diff" name="attach_280269" title="V3: notify the client asynchronously.">attachment 280269</a> <a href="attachment.cgi?id=280269&action=edit" title="V3: notify the client asynchronously.">[details]</a></span>
> V3: notify the client asynchronously.
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=280269&action=review">https://bugs.webkit.org/attachment.cgi?id=280269&action=review</a>
>
> > 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.</span >
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).
<span class="quote">>
> 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.</span >
Sure thing! I will make this change before landing.
<span class="quote">>
> r+ either way.</span >
Thank you for the review!</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>