Bug 143806 – Regression(r182517): WebSocket::suspend() causes error event to be fired

• Component: Page Loading
• Status: RESOLVED FIXED

[Chris Dumez]

WebSocket::suspend() causes error event to be fired after r182517. This is not allowed as firing the event could trigger arbitrary JS execution, which is no longer allowed at this point.
We should delay the error event firing until after WebSocket::resume() is called, similarly to what we already do for the close event.

Radar: <rdar://problem/20559812>

[Chris Dumez]

Created attachment 250892 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 250892 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:507
> +    for (auto& event : pendingEvents)
> +        dispatchEvent(WTF::move(event));

Anything can happen while firing the first event from pendingEvents, so some protection is likely needed.

At the very least, the WebSocket object should be protected with a Ref, but we also may need to check that we didn't enter suspend again. This gets confusing quickly.

[Chris Dumez]

Created attachment 250897 [details]
Patch

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 250892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250892&action=review
> 
> > Source/WebCore/Modules/websockets/WebSocket.cpp:507
> > +    for (auto& event : pendingEvents)
> > +        dispatchEvent(WTF::move(event));
> 
> Anything can happen while firing the first event from pendingEvents, so some
> protection is likely needed.
> 
> At the very least, the WebSocket object should be protected with a Ref, but
> we also may need to check that we didn't enter suspend again. This gets
> confusing quickly.

I protected |this| and handled the case where suspend() is called while we're firing the events (by queueing the events again).

[Alexey Proskuryakov]

Comment on attachment 250897 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:511
> +        dispatchOrQueueEvent(WTF::move(event));

I think that this results in a wrong order of events. The remaining events should be prepended (not one by one, but all a once).

It would be good to have test cases for these situations.

[Chris Dumez]

(In reply to comment #5)
> Comment on attachment 250897 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250897&action=review
> 
> > Source/WebCore/Modules/websockets/WebSocket.cpp:511
> > +        dispatchOrQueueEvent(WTF::move(event));
> 
> I think that this results in a wrong order of events. The remaining events
> should be prepended (not one by one, but all a once).
> 
> It would be good to have test cases for these situations.

Hmm. Good point. I'll see what happens if I navigate away from the error event handler.

[Chris Dumez]

Comment on attachment 250897 [details]
Patch

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

>>> Source/WebCore/Modules/websockets/WebSocket.cpp:511
>>> +        dispatchOrQueueEvent(WTF::move(event));
>> 
>> I think that this results in a wrong order of events. The remaining events should be prepended (not one by one, but all a once).
>> 
>> It would be good to have test cases for these situations.
> 
> Hmm. Good point. I'll see what happens if I navigate away from the error event handler.

I thought about this more and I don't think there can be any event in the Vector at this point. There is no API to reopen a WebSocket once it is closed. Therefore, the user would have to create a new WebSocket to reopen the connection.
For events to be queued, the WebSocket needs to be open when suspend() is called, which can only happen once.

I'll add a test for this. Also, creating a Vector copy here is not necessary. I can probably use a Dequeue and do something like this:
    while (!m_pendingEvents.isEmpty() && !m_shouldDelayEventFiring)
        dispatchEvent(m_pendingEvents.takeFirst());

[Chris Dumez]

Created attachment 250933 [details]
Patch

[Chris Dumez]

Comment on attachment 250933 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:509
> +    while (!m_pendingEvents.isEmpty() && !m_shouldDelayEventFiring)

Check for !m_shouldDelayEventFiring to stop iterating if suspend() is called while we fire the events. I am not sure this is possible but it will be handled if it is.

> Source/WebCore/Modules/websockets/WebSocket.cpp:520
> +    m_pendingEvents.clear();

We now clear the pending events in stop() so that resumeTimerFired() stops firing events if it is currently doing so.

> LayoutTests/ChangeLog:15
> +        * http/tests/websocket/tests/hybi/resources/page-cache-websocket.html: Added.

Added layout test to cover the case where WebSocket::stop() is called while firing the pending events upon restoring the page from PageCache.

I wasn't able to write one that causes WebSocket::suspend() to be called while firing the pending events as I wasn't able to trigger a synchronous navigation from the error event handler.

[Alexey Proskuryakov]

Comment on attachment 250933 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:471
> +    NoEventDispatchAssertion assertNoEventDispatch;

Don't we have a NoEventDispatchAssertion in the caller of suspend()?

> Source/WebCore/Modules/websockets/WebSocket.cpp:489
> +    NoEventDispatchAssertion assertNoEventDispatch;

Ditto.

[Chris Dumez]

Comment on attachment 250933 [details]
Patch

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

>> Source/WebCore/Modules/websockets/WebSocket.cpp:471
>> +    NoEventDispatchAssertion assertNoEventDispatch;
> 
> Don't we have a NoEventDispatchAssertion in the caller of suspend()?

ScriptExecutionContext::suspendActiveDOMObjects() is the call site. I don't see any NoEventDispatchAssertion there but it sounds like a good idea though. I'll look into this in a follow-up patch.

>> Source/WebCore/Modules/websockets/WebSocket.cpp:489
>> +    NoEventDispatchAssertion assertNoEventDispatch;
> 
> Ditto.

Ditto for ScriptExecutionContext::resumeActiveDOMObjects() call site.

There are assertions in place to make sure that ActiveDOMObjects are no removed or added. However, I don't see any check for event firing. Will look into this in a follow-up patch.

[WebKit Commit Bot]

Comment on attachment 250933 [details]
Patch

Rejecting attachment 250933 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 250933, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/5240127588663296

[Chris Dumez]

Created attachment 250936 [details]
Patch

[Build Bot]

Comment on attachment 250936 [details]
Patch

Attachment 250936 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5924837516115968

New failing tests:
compositing/scrolling/touch-scroll-to-clip.html

[Build Bot]

Created attachment 250938 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Chris Dumez]

Comment on attachment 250936 [details]
Patch

Clearing flags on attachment: 250936

Committed r182901: <http://trac.webkit.org/changeset/182901>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Looks like the new test is crashing on Windows:
https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r182909%20(51130)/results.html

However, there is no stack trace so I have no idea what's wrong with it.

[Alexey Proskuryakov]

Brent, is there any way to get a crash log more reliably? The test crashes almost every time.