Bug 158776

Summary: Use SocketProvider to create WebSocketChannels
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, buildbot, commit-queue, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 159768    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch none

Description Alex Christensen 2016-06-15 00:18:16 PDT
Use SocketProvider to create WebSocketChannels
Comment 1 Alex Christensen 2016-06-15 00:22:38 PDT
Created attachment 281343 [details]
Patch
Comment 2 Alex Christensen 2016-06-15 00:33:05 PDT
Created attachment 281344 [details]
Patch
Comment 3 Alex Christensen 2016-06-15 09:18:53 PDT
Created attachment 281364 [details]
Patch
Comment 4 WebKit Commit Bot 2016-06-15 09:20:09 PDT
Attachment 281364 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/Misc/WebSocketProvider.mm:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Build Bot 2016-06-15 10:09:16 PDT
Comment on attachment 281364 [details]
Patch

Attachment 281364 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1508104

New failing tests:
http/tests/websocket/construct-in-detached-frame.html
Comment 6 Build Bot 2016-06-15 10:09:18 PDT
Created attachment 281367 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 7 Alex Christensen 2016-06-15 10:15:25 PDT
Created attachment 281368 [details]
Patch
Comment 8 WebKit Commit Bot 2016-06-15 10:17:11 PDT
Attachment 281368 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/Misc/WebSocketProvider.mm:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Brent Fulgham 2016-06-16 16:19:17 PDT
Comment on attachment 281368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281368&action=review

This looks good to me. You need a WK2 review as well.

> Source/WebCore/ChangeLog:13
> +        be replaced by a proxy that will do the WebSocket operations in the NetworkProcess.

Yay!

> Source/WebCore/PlatformWin.cmake:170
> +    fileapi

Drive-by fix?

> LayoutTests/http/tests/websocket/construct-in-detached-frame-expected.txt:1
> +CONSOLE MESSAGE: line 10: InvalidStateError: DOM Exception 11

Nice!
Comment 10 Alex Christensen 2016-06-16 16:21:07 PDT
Comment on attachment 281368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281368&action=review

>> Source/WebCore/PlatformWin.cmake:170
>> +    fileapi
> 
> Drive-by fix?

No, this is needed to find a header that's included by a header that's now included in WebKit.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:653
> +		5C7706711D111B220012700F /* QuickDrawCompatibility.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = QuickDrawCompatibility.h; sourceTree = "<group>"; };

This is a drive-by fix, though.  This header wasn't included in the Xcode project.
Comment 11 Alex Christensen 2016-06-16 23:16:23 PDT
Created attachment 281537 [details]
Patch
Comment 12 WebKit Commit Bot 2016-06-16 23:18:49 PDT
Attachment 281537 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/Misc/WebSocketProvider.mm:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Alex Christensen 2016-06-16 23:19:21 PDT
(In reply to comment #9)
> > LayoutTests/http/tests/websocket/construct-in-detached-frame-expected.txt:1
> > +CONSOLE MESSAGE: line 10: InvalidStateError: DOM Exception 11
> 
> Nice!

This was actually a change in behavior that I remedied in my last patch.  We now initialize m_socketProvider in the Document constructor so that we have a SocketProvider if the Document had ever been attached to a Page through a Frame, not just if the Document was attached to a Page through a Frame the first time we needed to make a WebSocket.  I think a similar change should be made to Document::idbConnectionProxy so that detached iFrames can access IndexedDB, like http/tests/websocket/construct-in-detached-frame.html does with WebSockets.
Comment 14 Build Bot 2016-06-17 02:34:46 PDT
Comment on attachment 281537 [details]
Patch

Attachment 281537 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1517075

New failing tests:
fast/scrolling/ios/scrollTo-at-page-load.html
Comment 15 Build Bot 2016-06-17 02:34:50 PDT
Created attachment 281552 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 16 Alex Christensen 2016-06-17 22:01:44 PDT
Style bot is wrong - config.h isn't included in WebKit/mac.
ios-sim failure is unrelated.
win builds fine.  it's having trouble applying the patch that copies the files then changes them.

I think this is good and ready for review. r?
Comment 17 Brady Eidson 2016-06-20 15:51:37 PDT
Comment on attachment 281537 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281537&action=review

> Source/WebCore/Modules/websockets/WebSocket.cpp:259
> +    if (auto socketProvider = scriptExecutionContext()->socketProvider())
> +        m_channel = socketProvider->createWebSocketChannel(*scriptExecutionContext(), *this);
> +    if (!m_channel) {
> +        ec = INVALID_STATE_ERR;
> +        return;
> +    }

It's been described to me that this only happens with SVG documents.
And the inspector also has some empty clients for drawing overlays.

Neither of these are scriptable, so neither should ever hit the EC case above, so we should RELEASE_ASSERT_NOT_REACHED() there.

(Or RELEASE_ASSERT(m_channel))
Comment 18 Alex Christensen 2016-06-20 22:03:56 PDT
Created attachment 281712 [details]
Patch
Comment 19 WebKit Commit Bot 2016-06-20 22:05:28 PDT
Attachment 281712 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/Misc/WebSocketProvider.mm:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Brent Fulgham 2016-07-07 13:07:04 PDT
Comment on attachment 281712 [details]
Patch

It looks like you resolved the last batch of comments. The Window build failure seems unrelated to your change. r=me.
Comment 21 WebKit Commit Bot 2016-07-07 14:09:53 PDT
Comment on attachment 281712 [details]
Patch

Clearing flags on attachment: 281712

Committed r202930: <http://trac.webkit.org/changeset/202930>
Comment 22 WebKit Commit Bot 2016-07-07 14:09:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Alex Christensen 2016-07-07 15:38:21 PDT
Fixed Windows build in r202938