Bug 143806 - Regression(r182517): WebSocket::suspend() causes error event to be fired
Summary: Regression(r182517): WebSocket::suspend() causes error event to be fired
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 143505
  Show dependency treegraph
 
Reported: 2015-04-15 15:43 PDT by Chris Dumez
Modified: 2015-04-22 11:33 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.72 KB, patch)
2015-04-15 20:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (9.18 KB, patch)
2015-04-15 21:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (13.91 KB, patch)
2015-04-16 11:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (13.91 KB, patch)
2015-04-16 12:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (522.26 KB, application/zip)
2015-04-16 12:39 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-04-15 15:43:05 PDT
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>
Comment 1 Chris Dumez 2015-04-15 20:10:09 PDT
Created attachment 250892 [details]
Patch
Comment 2 Alexey Proskuryakov 2015-04-15 20:53:49 PDT
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.
Comment 3 Chris Dumez 2015-04-15 21:29:03 PDT
Created attachment 250897 [details]
Patch
Comment 4 Chris Dumez 2015-04-15 21:30:34 PDT
(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).
Comment 5 Alexey Proskuryakov 2015-04-15 21:53:03 PDT
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.
Comment 6 Chris Dumez 2015-04-15 22:08:50 PDT
(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.
Comment 7 Chris Dumez 2015-04-16 10:10:35 PDT
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());
Comment 8 Chris Dumez 2015-04-16 11:42:18 PDT
Created attachment 250933 [details]
Patch
Comment 9 Chris Dumez 2015-04-16 11:45:40 PDT
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.
Comment 10 Alexey Proskuryakov 2015-04-16 11:55:17 PDT
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.
Comment 11 Chris Dumez 2015-04-16 12:02:26 PDT
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.
Comment 12 WebKit Commit Bot 2015-04-16 12:04:36 PDT
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
Comment 13 Chris Dumez 2015-04-16 12:06:26 PDT
Created attachment 250936 [details]
Patch
Comment 14 Build Bot 2015-04-16 12:39:12 PDT
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
Comment 15 Build Bot 2015-04-16 12:39:16 PDT
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
Comment 16 Chris Dumez 2015-04-16 12:40:50 PDT
Comment on attachment 250936 [details]
Patch

Clearing flags on attachment: 250936

Committed r182901: <http://trac.webkit.org/changeset/182901>
Comment 17 Chris Dumez 2015-04-16 12:40:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Chris Dumez 2015-04-16 15:15:00 PDT
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.
Comment 19 Alexey Proskuryakov 2015-04-16 21:19:26 PDT
Brent, is there any way to get a crash log more reliably? The test crashes almost every time.