Closed Bug 1132537 Opened 5 years ago Closed 5 years ago

WebSocketImpl::Dispatch uses some non-mutex-protected variables

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch atomic.patch (obsolete) — Splinter Review
Attachment #8564119 - Flags: review?(bugs)
Comment on attachment 8564119 [details] [diff] [review]
atomic.patch

Why is this enough?
Don't we need to have the lock, and then based on the state of the boolean flag
do something, and release the lock.
Otherwise we may check the flag, some other thread changes the flag, and the original thread continues doing something.
Attachment #8564119 - Flags: review?(bugs) → review-
> Why is this enough?

I think so. We are protecting 2 variables:

1. mWorkerShuttingDown - this is set to true from the feature without checking the previous state. Then it's used by Dispatch() to avoid to send messages when the worker is shutting down. We don't want to lock the whole operation of dispatching events, so we still have the possibility to pass the check test and then to dispatch a message when it's too late.
This is still fine because we will receive a warning message and the runnable will not be dispatched.
Plus, it's possible that, when the worker starts shutting down, it already have some runnable in the event queue.

2. For mDisconnectedOrDisconnecting, I found a wonderful mDisconnectedOrDisconnecting.exchange(true) that does the magic for me. I'll submit a new patch.
Attached patch atomic.patch (obsolete) — Splinter Review
Attachment #8564119 - Attachment is obsolete: true
Attachment #8564496 - Flags: review?(bugs)
So
http://mxr.mozilla.org/mozilla-central/source/dom/base/WebSocket.cpp?rev=20729b28eb1e#2605
relies on mWorkerShuttingDown to be set true only in the same thread where the check is done.
If so, why does mWorkerShuttingDown need to be atomic? And if not, why is atomic enough?


Are all the other use of mDisconnectingOrDisconnected safe? What if the value is first 
false, and just after 'if (mDisconnectingOrDisconnected)' check some other threads changes it to
true?
Though, apparently mDisconnectingOrDisconnected is changed to true only in target thread and we do
have assertions in those methods doing 'if (mDisconnectingOrDisconnected)'.
> If so, why does mWorkerShuttingDown need to be atomic? And if not, why is
> atomic enough?

Dispatch() can run in any thread. And maybe we are checking mWorkerShuttingDown in a different thread there.

> Are all the other use of mDisconnectingOrDisconnected safe? What if the
> value is first 
> false, and just after 'if (mDisconnectingOrDisconnected)' check some other
> threads changes it to
> true?

That should be fine because mDisconnectingOrDisconneceted and mWorkerShuttingDown are used as a pre-check. If we dispatch a runnable and the worker is already gone we fail dispatch the runnable. No big deal. If the runnable does any operation with WebSocketImpl we check mDisconnectingOrDisconnected everywhere on the target thread.
Why do we have the check for mWorkerShuttingDown then, if it isn't actually needed?
Well, because otherwise we call Dispatch() in the worker and there we have a warning. We know that the worker is shutting down because we have the notification from the feature, and we use this info.
I would then just remove mWorkerShuttingDown from Dispatch, if all it is for is to prevent a warning in a racy case, and elsewhere mWorkerShuttingDown is used on target thread only, right?
Well... no, because at some point the worker will shutdown completely and at that time we should dispatch new runnables. That is why we use that variable in Dispatch().
...and at that time we should dispatch ... ?

What guarantees that  between 'if (mWorkerShuttingDown)' and
if (!event->Dispatch(nullptr)) { the worker isn't shutdown?
Attached patch atomic.patch (obsolete) — Splinter Review
Attachment #8564496 - Attachment is obsolete: true
Attachment #8564496 - Flags: review?(bugs)
Attachment #8565004 - Flags: review?(bugs)
What happens if mDisconnectingOrDisconnected is set to true after 
if (mDisconnectingOrDisconnected) { in Disconnect() ?
Any other method has a check for that.
So why we need mDisconnectingOrDisconnected then in Dispatch()?
Attached patch atomic.patch (obsolete) — Splinter Review
Attachment #8565004 - Attachment is obsolete: true
Attachment #8565004 - Flags: review?(bugs)
Attachment #8565151 - Flags: review?(bugs)
Attached patch atomic.patchSplinter Review
Sorry, wrong patch.
Attachment #8565151 - Attachment is obsolete: true
Attachment #8565151 - Flags: review?(bugs)
Attachment #8565153 - Flags: review?(bugs)
Comment on attachment 8565153 [details] [diff] [review]
atomic.patch

Not clear to me why WorkerRunnableDispatcher needs to be created earlier, but should be fine.
Attachment #8565153 - Flags: review?(bugs) → review+
I did it just to reduce the locking time.
https://hg.mozilla.org/mozilla-central/rev/b87c58ef8667
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.