Bug 159680 - [WK1][iOS] Crash when WebSocket attempts to dispatch a mixed content blocker event
Summary: [WK1][iOS] Crash when WebSocket attempts to dispatch a mixed content blocker ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-12 10:39 PDT by Brent Fulgham
Modified: 2016-07-13 14:01 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.00 KB, patch)
2016-07-12 10:42 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (6.04 KB, patch)
2016-07-12 16:52 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.88 KB, patch)
2016-07-12 17:56 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.80 KB, patch)
2016-07-12 18:00 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.67 KB, patch)
2016-07-12 18:07 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2016-07-12 10:39:15 PDT
WebSockets on WK1 will sometimes crash with a null pointer exception when attempting to dispatch a mixed content error event.
Comment 1 Brent Fulgham 2016-07-12 10:39:44 PDT
<rdar://problem/22102028>
Comment 2 Brent Fulgham 2016-07-12 10:42:00 PDT
Created attachment 283426 [details]
Patch
Comment 3 Brent Fulgham 2016-07-12 10:42:36 PDT
I'm working on a suitable LayoutTest, but wanted to get feedback on the use of WebThreadRun.
Comment 4 Anders Carlsson 2016-07-12 10:54:54 PDT
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.
Comment 5 Brent Fulgham 2016-07-12 16:52:16 PDT
Created attachment 283470 [details]
Patch
Comment 6 Simon Fraser (smfr) 2016-07-12 16:56:57 PDT
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?
Comment 7 Brent Fulgham 2016-07-12 17:40:10 PDT
(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.
Comment 8 Brent Fulgham 2016-07-12 17:40:41 PDT
(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).
Comment 9 Simon Fraser (smfr) 2016-07-12 17:47:45 PDT
Can you just use a zero-delay timer?
Comment 10 Brent Fulgham 2016-07-12 17:56:28 PDT
Created attachment 283480 [details]
Patch
Comment 11 Andy Estes 2016-07-12 17:57:15 PDT
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?
Comment 12 Brent Fulgham 2016-07-12 18:00:18 PDT
(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.
Comment 13 Brent Fulgham 2016-07-12 18:00:46 PDT
Created attachment 283481 [details]
Patch
Comment 14 Andy Estes 2016-07-12 18:04:53 PDT
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.
Comment 15 Brent Fulgham 2016-07-12 18:07:34 PDT
Created attachment 283483 [details]
Patch
Comment 16 Andy Estes 2016-07-12 18:16:42 PDT
Comment on attachment 283483 [details]
Patch

Wait, couldn't this be called on a worker thread?
Comment 17 Andy Estes 2016-07-12 18:27:43 PDT
(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.
Comment 18 Brent Fulgham 2016-07-12 20:05:40 PDT
(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. :-)
Comment 19 Brent Fulgham 2016-07-12 20:15:26 PDT
Based on Andy's comments, moving the "dispatch_async(main) + WebThreadRun" version of the patch for approval.
Comment 20 Andy Estes 2016-07-13 10:21:34 PDT
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 21 WebKit Commit Bot 2016-07-13 10:26:15 PDT
Comment on attachment 283470 [details]
Patch

Clearing flags on attachment: 283470

Committed r203165: <http://trac.webkit.org/changeset/203165>
Comment 22 WebKit Commit Bot 2016-07-13 10:26:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Michael Catanzaro 2016-07-13 14:01:02 PDT
FYI there's probably another crash in this code, bug #149864.