Bug 158776 – Use SocketProvider to create WebSocketChannels

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Use SocketProvider to create WebSocketChannels

[Alex Christensen]

Created attachment 281343 [details]
Patch

[Alex Christensen]

Created attachment 281344 [details]
Patch

[Alex Christensen]

Created attachment 281364 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Alex Christensen]

Created attachment 281368 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

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!

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 281537 [details]
Patch

[WebKit Commit Bot]

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.

[Alex Christensen]

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

[Build Bot]

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

[Build Bot]

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

[Alex Christensen]

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?

[Brady Eidson]

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

[Alex Christensen]

Created attachment 281712 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

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.

[WebKit Commit Bot]

Comment on attachment 281712 [details]
Patch

Clearing flags on attachment: 281712

Committed r202930: <http://trac.webkit.org/changeset/202930>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

Fixed Windows build in r202938