1445681 - EventSource can fail the shutting down when used in a terminating worker

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

When the Web Worker goes away, EventSource holder is notified:
https://searchfox.org/mozilla-central/source/dom/base/EventSource.cpp#1795,1799

At this point, we dispatch a runnable to call CloseInternal:
https://searchfox.org/mozilla-central/source/dom/base/EventSource.cpp#394-399

This line: MOZ_ASSERT(NS_SUCCEEDED(rv));
is wrong, because we end up using a normal WorkerRunnable and that will not be executed during the shutdown.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: eventSource.patch

I don't think we need this async step to call CloseInternal. There is not a DOM Event dispatched as the comment says. We can directly call CloseInternal.
I hope I'm not wrong. I also checked the spec and my approach seems correct.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Isn't the comment about some event listener calling close().
But I still don't quite see why the code is there. Bug 1267903 doesn't really explain it.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

But this code looks very racy.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

IsClosed() check in particular. Since it can be called without holding the lock, even if it returns false, the state may have changed to true already when it returns.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8958874 [details] [diff] [review]
eventSource.patch

However as far as I see, this as such should be fine.

But could you take a look at the implementation a bit. I'm worried about IsClosed() handling. I think that method should take MutexAutoLock& as a param as a gaurantee that caller has lock.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b12a2c571c9
EventSource::close must do a sync call to CloseInternal, r=smaug

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6b12a2c571c9