WebSockets on WK1 will sometimes crash with a null pointer exception when attempting to dispatch a mixed content error event.
<rdar://problem/22102028>
Created attachment 283426 [details] Patch
I'm working on a suitable LayoutTest, but wanted to get feedback on the use of WebThreadRun.
Comment on attachment 283426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283426&action=review > Source/WebCore/Modules/websockets/WebSocket.cpp:303 > + WebThreadRun(^{ > + dispatchOrQueueErrorEvent(); > + stop(); > + }); This doesn't protect the socket.
Created attachment 283470 [details] Patch
Comment on attachment 283470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283470&action=review > Source/WebCore/Modules/websockets/WebSocket.cpp:307 > + dispatch_async(dispatch_get_main_queue(), ^{ > + WebThreadRun(^{ > + dispatchOrQueueErrorEvent(); > + stop(); > + deref(); > + }); > + }); WebThreadRun is already async. Why wrap it in a dispatch_async?
(In reply to comment #6) > Comment on attachment 283470 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283470&action=review > > > Source/WebCore/Modules/websockets/WebSocket.cpp:307 > > + dispatch_async(dispatch_get_main_queue(), ^{ > > + WebThreadRun(^{ > > + dispatchOrQueueErrorEvent(); > > + stop(); > > + deref(); > > + }); > > + }); > > WebThreadRun is already async. Why wrap it in a dispatch_async? See the comment on lines 295-299. This WebSocket constructor gets created on the WebThread (at least in iOS/WK1). So calling WebThreadRun immediately begins executing the block, which is not what we want. Instead, we ask the main queue to enqueue the block in the WebThread during the next run loop. It might be cleaner to modify the WebThread API so that I could just enqueue the block in the run queue without executing the code here. E.g., What I really want here is for the block I'm sending to WebThreadRun to be executed on the NEXT run loop. I could do that if I could add it to the runQueue member in WebCore/platform/ios/wak/WebCoreThreadRun.cpp.
(In reply to comment #7) > (In reply to comment #6) > > Comment on attachment 283470 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=283470&action=review > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:307 > > > + dispatch_async(dispatch_get_main_queue(), ^{ > > > + WebThreadRun(^{ > > > + dispatchOrQueueErrorEvent(); > > > + stop(); > > > + deref(); > > > + }); > > > + }); > > > > WebThreadRun is already async. Why wrap it in a dispatch_async? > > See the comment on lines 295-299. > > This WebSocket constructor gets created on the WebThread (at least in > iOS/WK1). So calling WebThreadRun immediately begins executing the block, > which is not what we want. > > Instead, we ask the main queue to enqueue the block in the WebThread during > the next run loop. > > It might be cleaner to modify the WebThread API so that I could just enqueue > the block in the run queue without executing the code here. > > E.g., What I really want here is for the block I'm sending to WebThreadRun > to be executed on the NEXT run loop. I could do that if I could add it to > the runQueue member in WebCore/platform/ios/wak/WebCoreThreadRun.cpp. Andy: Can you recommend a way to do what I want (see my above comment).
Can you just use a zero-delay timer?
Created attachment 283480 [details] Patch
Comment on attachment 283480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283480&action=review > Source/WebCore/Modules/websockets/WebSocket.cpp:304 > +#if USE(WEB_THREAD) > + ASSERT(WebThreadIsCurrent()); > + RunLoop::current().dispatch([this, protectedThis = Ref<WebSocket>(*this)]() { > +#else > RunLoop::main().dispatch([this, protectedThis = Ref<WebSocket>(*this)]() { > +#endif Won't RunLoop::current() work even int he non-WebThread case?
(In reply to comment #11) > Comment on attachment 283480 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283480&action=review > > > Source/WebCore/Modules/websockets/WebSocket.cpp:304 > > +#if USE(WEB_THREAD) > > + ASSERT(WebThreadIsCurrent()); > > + RunLoop::current().dispatch([this, protectedThis = Ref<WebSocket>(*this)]() { > > +#else > > RunLoop::main().dispatch([this, protectedThis = Ref<WebSocket>(*this)]() { > > +#endif > > Won't RunLoop::current() work even int he non-WebThread case? It seems to. I'll revise the patch.
Created attachment 283481 [details] Patch
Comment on attachment 283481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283481&action=review > Source/WebCore/Modules/websockets/WebSocket.cpp:68 > +#if USE(WEB_THREAD) > +#include "WebCoreThreadRun.h" > +#endif This shouldn't be needed anymore. > Source/WebCore/Modules/websockets/WebSocket.cpp:299 > - RunLoop::main().dispatch([this, protectedThis = Ref<WebSocket>(*this)]() { > + RunLoop::current().dispatch([this, protectedThis = Ref<WebSocket>(*this)]() { An ASSERT(isMainThread()) (which really means is main-or-web thread) would be nice here.
Created attachment 283483 [details] Patch
Comment on attachment 283483 [details] Patch Wait, couldn't this be called on a worker thread?
(In reply to comment #16) > Comment on attachment 283483 [details] > Patch > > Wait, couldn't this be called on a worker thread? If so, then your solution of dispatching to the main thread then calling WebThreadRun() seems ok to me. You'd only be penalizing iOS WebKit1 with a second dispatch.
(In reply to comment #17) > (In reply to comment #16) > > Comment on attachment 283483 [details] > > Patch > > > > Wait, couldn't this be called on a worker thread? > > If so, then your solution of dispatching to the main thread then calling > WebThreadRun() seems ok to me. You'd only be penalizing iOS WebKit1 with a > second dispatch. Yes -- its possible for these WebSockets to be used by workers, so I'll revert back to my original patch. :-)
Based on Andy's comments, moving the "dispatch_async(main) + WebThreadRun" version of the patch for approval.
While looking this over I noticed that mixed content checking is bypassed for WebSockets that that don't have Documents. For instance, it's possible to communicate using the ws: protocol from inside a Worker on a secure page. I filed <https://bugs.webkit.org/show_bug.cgi?id=159726> about this.
Comment on attachment 283470 [details] Patch Clearing flags on attachment: 283470 Committed r203165: <http://trac.webkit.org/changeset/203165>
All reviewed patches have been landed. Closing bug.
FYI there's probably another crash in this code, bug #149864.