Bug 159680 – [WK1][iOS] Crash when WebSocket attempts to dispatch a mixed content blocker event

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

WebSockets on WK1 will sometimes crash with a null pointer exception when attempting to dispatch a mixed content error event.

[Brent Fulgham]

<rdar://problem/22102028>

[Brent Fulgham]

Created attachment 283426 [details]
Patch

[Brent Fulgham]

I'm working on a suitable LayoutTest, but wanted to get feedback on the use of WebThreadRun.

[Anders Carlsson]

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.

[Brent Fulgham]

Created attachment 283470 [details]
Patch

[Simon Fraser (smfr)]

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?

[Brent Fulgham]

(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.

[Brent Fulgham]

(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).

[Simon Fraser (smfr)]

Can you just use a zero-delay timer?

[Brent Fulgham]

Created attachment 283480 [details]
Patch

[Andy Estes]

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?

[Brent Fulgham]

(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.

[Brent Fulgham]

Created attachment 283481 [details]
Patch

[Andy Estes]

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.

[Brent Fulgham]

Created attachment 283483 [details]
Patch

[Andy Estes]

Comment on attachment 283483 [details]
Patch

Wait, couldn't this be called on a worker thread?

[Andy Estes]

(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.

[Brent Fulgham]

(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. :-)

[Brent Fulgham]

Based on Andy's comments, moving the "dispatch_async(main) + WebThreadRun" version of the patch for approval.

[Andy Estes]

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.

[WebKit Commit Bot]

Comment on attachment 283470 [details]
Patch

Clearing flags on attachment: 283470

Committed r203165: <http://trac.webkit.org/changeset/203165>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

FYI there's probably another crash in this code, bug #149864.