1267903 - EventSource for workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P2)

Description

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

Looks like at least some browsers support it in workers, and per spec we should too.

Comment 1

[Andrew Overholt [:overholt]]

Is this more urgent than backlog?

Comment 2

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

Not particularly urgent, but we're shipping EventSource on main thread only, when some other implementations follow the spec and ship it on workers too.

Comment 3

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

This might be nice bug for someone to learn more about workers' code.
(and I'm sure baku would be happy to mentor and review here ;) )

Comment 4

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

Someone just sent email to [e-mail] complaining that Chrome supports EventSource in ServiceWorkers (and Workers) but Firefox doesn't.

Comment 5

[Sam Ruby]

That would be me.

https://lists.w3.org/Archives/Public/public-webapps/2016JulSep/0016.html

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Is Bug 876498 a duplicate of this? 

Set to P2 as I am working on making it planned.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Stone is willing to learn more about Workers :)

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Note, there is a spec question about if this should be exposed in SW:

https://github.com/w3c/ServiceWorker/issues/947

Might want to get that clarified before progressing here.

Comment 10

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

Well, I'd say we should expose. If WebSocket is there, EventSource should too.
(IMO, also XHR, but others may disagree.)

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Well, if we exposed it today it would not spawn the service worker and it would not hold the worker alive.  I doubt that is what any user of this API would expect.

Comment 12

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

I would expect waitUntil to be used here. Just the same thing what you mentioned would be used for sub-workers, no?

Comment 13

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

(I could totally misunderstand the meaning of waitUntil. But if that doesn't work here, why would it work with sub-workers in any sane way?)

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

You could use waitUntil() sure, but I bet it still confuses devs.  EventSource is designed as a long duration thing to stream many events with stuff like auto reconnection.  This doesn't fit with event.waitUntil() very well.

Anyway, best to take your views to the linked github spec issue.

Comment 15

[Sam Ruby]

Testing with WebSockets... and Ben's comment about not holding the worker alive and not being what any user of the API would expect applies here: a short while after the service worker is activated it goes away and the socket is closed.

That appears to be intentional, see Jaffa the Cake's comment here: http://stackoverflow.com/questions/29741922/prevent-service-worker-from-automatically-stopping

Unfortunately, my use case is exactly the one that falls through the cracks: 'The "silent" push use case is not supported right now.'  I'm going to explore shared workers, but the future of that seems uncertain with Apple deprecating then removing the feature from Safari.  Not that Apple supports Service Workers, but the momentum seems to be heading in that direction.  Microsoft has listed Service Workers as in Development, but has Shared Web Workers as not currently planned.  :-(

Comment 16

[Ming-Chou Shih [:stone]]

Attachment: Bug 1267903 Part1: Implement EventSource for Worker.

Comment 17

[Ming-Chou Shih [:stone]]

Attachment: Bug 1267903 Part2: Enable related test cases.

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8814345 [details] [diff] [review]
Bug 1267903 Part1: Implement EventSource for Worker.

Review of attachment 8814345 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Some comments:

1. you should support workers without windows.

2.  You should check if you can do:
  nsCOMPtr<nsIThreadRetargetableRequest> rr = do_QueryInterface(httpChannel);
  rr->RetargetDeliveryTo(this);
and in this way, http should be able to dispatch OnDataAvailable to the correct thread.

::: dom/base/EventSource.cpp
@@ +45,5 @@
>  #include "nsWrapperCacheInlines.h"
>  #include "mozilla/Attributes.h"
>  #include "nsError.h"
>  
> +using namespace mozilla::dom::workers;

move this into namespace mozilla { namespace dom {

as:

using namespace workers;

@@ +95,5 @@
> +  void Close();
> +
> +  void Init(nsIPrincipal* aPrincipal, const nsAString& aURL, ErrorResult& aRv);
> +
> +  nsresult GetBaseURI(nsIURI **aBaseURI);

nsIURI** aBaseURI

@@ +114,5 @@
> +
> +  nsresult Thaw();
> +  nsresult Freeze();
> +
> +  static void TimerCallback(nsITimer *aTimer, void *aClosure);

in general, type* aValue
Everywhere in this code.

@@ +147,5 @@
> +
> +  bool RegisterWorkerHolder();
> +  void UnregisterWorkerHolder();
> +
> +  void AssertIsOnTargetThread() const { MOZ_ASSERT(IsTargetThread()); }

I like more:

void AssertIsOnTargetThread() const
{
  MOZ_ASSERT(IsTargetThread()); }
}

also in the other methods.

@@ +161,5 @@
> +  {
> +    MOZ_ASSERT(mEventSource);
> +    MutexAutoLock lock(mMutex);
> +    mReadyState = aReadyState;
> +    mEventSource->mReadyState = aReadyState;

why mReadyState and mEventSource->mReadyState?

can we just have 1 of them?

@@ +181,5 @@
> +  {
> +    return ReadyState() == CLOSED;
> +  }
> +
> +  RefPtr<EventSource> mEventSource;

Write a comment next to each member saying if it's protected by mutex.

@@ +275,5 @@
> +  WorkerPrivate* mWorkerPrivate;
> +  // Holder to worker to keep worker alive. (accessed on worker thread only)
> +  nsAutoPtr<WorkerHolder> mWorkerHolder;
> +  // This mutex protects mFrozen that are used in different threads.
> +  mozilla::Mutex mMutex;

and mReadyState, right?

@@ +345,5 @@
> +  , mDispatchingDOMEventCount(0)
> +  , mScriptLine(0)
> +  , mScriptColumn(0)
> +  , mInnerWindowID(0)
> +{

MOZ_ASSERT(mEventSource);

@@ +350,5 @@
> +  if (!NS_IsMainThread()) {
> +    mWorkerPrivate = GetCurrentThreadWorkerPrivate();
> +    MOZ_ASSERT(mWorkerPrivate);
> +    mIsMainThread = false;
> +    MOZ_ASSERT(mEventSource);

remove this.

@@ +413,5 @@
> +  if (NS_IsMainThread()) {
> +    Cleanup();
> +  } else {
> +    ErrorResult rv;
> +    RefPtr<CleanupRunnable> runnable = new CleanupRunnable(this);

Write a comment saying that this runnable is sync.

@@ +427,5 @@
> +  RefPtr<EventSourceImpl> kungfuDeathGrip = this;
> +  mEventSource->UpdateDontKeepAlive();
> +}
> +
> +void EventSourceImpl::Cleanup()

rename this to CleanUpMainThread, or something similar.

@@ +434,2 @@
>    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +  MOZ_ASSERT(os);

remove this assertion :)
When shutting down, obs can be null.

@@ +472,5 @@
> +    nsCOMPtr<nsIPrincipal> principal;
> +    while (wp->GetParent()) {
> +      wp = wp->GetParent();
> +    }
> +    nsPIDOMWindowInner* window = wp->GetWindow();

principal = window->GetPrincipal();
if (!principal) {
  ...

@@ +484,5 @@
> +    } else {
> +      principal = wp->GetPrincipal();
> +    }
> +    if (!principal) {
> +      mRv.Throw(NS_ERROR_FAILURE);

ErrorResult are not thread-safe. This is a bug also in WebSocket code. I'll fix it.

@@ +501,4 @@
>  
> +nsresult
> +EventSourceImpl::ParseURL(const nsAString& aURL)
> +{

Which thread are we? Put an assertion.
Do it for each method. If the method can be called on any thread, write it in a comment.

@@ +544,5 @@
> +EventSourceImpl::Init(nsIPrincipal* aPrincipal,
> +                      const nsAString& aURL,
> +                      ErrorResult& aRv)
> +{
> +  AssertIsOnMainThread();

MOZ_ASSERT(aPrincipal)

@@ +623,5 @@
>  //-----------------------------------------------------------------------------
>  
>  NS_IMETHODIMP
> +EventSourceImpl::OnStartRequest(nsIRequest *aRequest,
> +                                nsISupports *ctxt)

aCtxt

@@ +683,5 @@
> +    NS_ENSURE_STATE(mStream);
> +    rv = mStream->AsyncWait(/*aCallback */ this,
> +                            /* aFlags*/ 0,
> +                            /* aRequestedCount */ 0,
> +                            /* aTarget*/ this);

Why all of this?

@@ +742,5 @@
>    if (!thisObject || !aWriteCount) {
>      NS_WARNING("EventSource cannot read from stream: no aClosure or aWriteCount");
>      return NS_ERROR_FAILURE;
>    }
> +  MOZ_ASSERT(NS_IsMainThread() == thisObject->mIsMainThread);

thisObject->IsTargetThread() ?

@@ +783,2 @@
>  {
> +  AssertIsOnMainThread();

Cannot we change the target for dataAvailable?

@@ +806,1 @@
>                             nsISupports *aContext,

indentation

::: dom/base/EventSource.h
@@ +55,3 @@
>  
>    // nsWrapperCache
> +  nsPIDOMWindowInner* GetParentObject() const { return GetOwner(); }

Get rid of this. DOMEventTargetHelper has its own GetParentObject()

@@ +89,4 @@
>    void Close();
>  
> +private:
> +  explicit EventSource(nsPIDOMWindowInner* aOwnerWindow, bool aWithCredentials);

no explicit when more than 1 argument.

@@ +104,2 @@
>  
> +  EventSourceImpl* mImpl;

Write a comment about what keeps this object alive.

Comment 19

[Ming-Chou Shih [:stone]]

(In reply to Andrea Marchesini [:baku] from comment #18)
> 1. you should support workers without windows.
Could you kindly explain more about this part? Sorry for that I have no idea about the required steps to support worker without windows. Is it related to get the correct principal from worker or something else?

> 2.  You should check if you can do:
>   nsCOMPtr<nsIThreadRetargetableRequest> rr = do_QueryInterface(httpChannel);
>   rr->RetargetDeliveryTo(this);
> and in this way, http should be able to dispatch OnDataAvailable to the
> correct thread.
It's because HttpChannelChild doesn't implement nsIThreadRetargetableRequest and I can't dispatch OnDataAvailable on the target thread.

> @@ +161,5 @@
> > +  {
> > +    MOZ_ASSERT(mEventSource);
> > +    MutexAutoLock lock(mMutex);
> > +    mReadyState = aReadyState;
> > +    mEventSource->mReadyState = aReadyState;
> 
> why mReadyState and mEventSource->mReadyState?
> 
> can we just have 1 of them?
When closing EventSource, EventSourceImpl may be waiting for the notifications from the channel and the notifications may come after EventSource is destroyed. So I keep a copy of the mReadyState in EventSourceImpl to know whether it's closed. Or I could replace it with a boolean to indicate it's closed. How do you think?

> @@ +683,5 @@
> > +    NS_ENSURE_STATE(mStream);
> > +    rv = mStream->AsyncWait(/*aCallback */ this,
> > +                            /* aFlags*/ 0,
> > +                            /* aRequestedCount */ 0,
> > +                            /* aTarget*/ this);
> 
> Why all of this?
I try to use pipe to transport data and parse data on target thread because I can't dispatch OnDataAvailable on the target thread. I had some surveys and discussions about implementing nsIThreadRetargetableRequest for HttpChannelChild but so far it's hard for me because of my limited understandings to Necko. I have no idea if there are simpler solutions for this problem and would like to learn from you. Thanks.

Comment 20

[Andrea Marchesini [:baku]]

I filed bug 1320744 for having HttpChannelChild implementing nsIThreadRetargetableRequest interface.

Comment 21

[Andrea Marchesini [:baku]]

I think it's better to have nsIThreadRetargetableRequest interface in HttpChannelChild.
Let's see if somebody from the necko team wants to work on it. Or if you want, you can take that bug as well :)

Comment 22

[Ming-Chou Shih [:stone]]

(In reply to Andrea Marchesini [:baku] from comment #21)
> I think it's better to have nsIThreadRetargetableRequest interface in
> HttpChannelChild.
> Let's see if somebody from the necko team wants to work on it. Or if you
> want, you can take that bug as well :)
Many thanks for your help. So far I have no confidence to take it but I'll study more about it at the same time.

Comment 23

[Andrea Marchesini [:baku]]

> > 1. you should support workers without windows.

Actually, I think your patch is just fine. Maybe add a test using EventSource in a SharedWorker.

> When closing EventSource, EventSourceImpl may be waiting for the
> notifications from the channel and the notifications may come after
> EventSource is destroyed. So I keep a copy of the mReadyState in
> EventSourceImpl to know whether it's closed. Or I could replace it with a
> boolean to indicate it's closed. How do you think?

EventSourceImpl keeps EventSource alive. If mEventSource is null, it means that the EventSource has been closed.

Comment 24

[Ming-Chou Shih [:stone]]

Attachment: Part1: Implement EventSource for Worker.

Followed feedbacks to refine the patch

Comment 25

[Ming-Chou Shih [:stone]]

Attachment: Enable related test cases

Followed feedbacks to refine the patch

Comment 26

[Ming-Chou Shih [:stone]]

Attachment: Part1: Implement EventSource for Worker.

Comment 27

[Ming-Chou Shih [:stone]]

Attachment: Part2: Enable related test cases.

Comment 28

[Ming-Chou Shih [:stone]]

(In reply to Andrea Marchesini [:baku] from comment #23)
> > > 1. you should support workers without windows.
> 
> Actually, I think your patch is just fine. Maybe add a test using
> EventSource in a SharedWorker.
I found there are some web platform tests (in eventsource/shared-worker) testing using EventSource in a SharedWorker so enabled them in the part2 patch.

Comment 29

[Andrea Marchesini [:baku]]

Comment on attachment 8821948 [details] [diff] [review]
Part1: Implement EventSource for Worker.

Review of attachment 8821948 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/EventSource.cpp
@@ +159,5 @@
> +  uint16_t ReadyState()
> +  {
> +    if (mEventSource) {
> +      MutexAutoLock lock(mMutex);
> +      return mEventSource->mReadyState;      

extra space.

@@ +257,5 @@
> +  nsCOMPtr<nsITimer> mTimer;
> +  nsCOMPtr<nsILoadGroup> mLoadGroup;
> +  nsCOMPtr<nsIHttpChannel> mHttpChannel;
> +
> +  struct Message {

{ in a new line.

@@ +327,5 @@
> +                  nsIThreadRetargetableStreamListener)
> +
> +EventSourceImpl::EventSourceImpl(EventSource* aEventSource)
> +  : mEventSource(aEventSource)
> +  , mSrc(nullptr)

remove

@@ +329,5 @@
> +EventSourceImpl::EventSourceImpl(EventSource* aEventSource)
> +  : mEventSource(aEventSource)
> +  , mSrc(nullptr)
> +  , mReconnectionTime(0)
> +  , mPrincipal(nullptr)

not needed

@@ +330,5 @@
> +  : mEventSource(aEventSource)
> +  , mSrc(nullptr)
> +  , mReconnectionTime(0)
> +  , mPrincipal(nullptr)
> +  , mTimer(nullptr)

not needed

@@ +331,5 @@
> +  , mSrc(nullptr)
> +  , mReconnectionTime(0)
> +  , mPrincipal(nullptr)
> +  , mTimer(nullptr)
> +  , mLoadGroup(nullptr)

this is not needed

@@ +332,5 @@
> +  , mReconnectionTime(0)
> +  , mPrincipal(nullptr)
> +  , mTimer(nullptr)
> +  , mLoadGroup(nullptr)
> +  , mHttpChannel(nullptr)

Not needed

@@ +334,5 @@
> +  , mTimer(nullptr)
> +  , mLoadGroup(nullptr)
> +  , mHttpChannel(nullptr)
> +  , mStatus(PARSE_STATE_OFF)
> +  , mUnicodeDecoder(nullptr)

remove this

@@ +337,5 @@
> +  , mStatus(PARSE_STATE_OFF)
> +  , mUnicodeDecoder(nullptr)
> +  , mLastConvertionResult(NS_OK)
> +  , mWorkerPrivate(nullptr)
> +  , mWorkerHolder(nullptr)

not needed

@@ +341,5 @@
> +  , mWorkerHolder(nullptr)
> +  , mMutex("EventSourceImpl::mMutex")
> +  , mFrozen(false)
> +  , mGoingToDispatchAllMessages(false)
> +  , mIsMainThread(true)

mIsMainThread(NS_IsMainThread())

@@ +347,5 @@
> +  , mScriptColumn(0)
> +  , mInnerWindowID(0)
> +{
> +  MOZ_ASSERT(mEventSource);
> +  if (!NS_IsMainThread()) {

if (!mIsMainThread) {

@@ +385,5 @@
> +  if (ReadyState() == CLOSED) {
> +    return;
> +  }
> +  if (!IsTargetThread()) {
> +    Dispatch(NewRunnableMethod(this, &EventSourceImpl::Close),

This is wrong because you are calling EventSourceImpl::Close() in the target thread asynchronously, but few lines after you set the state to CLOSED. When this method is called on the worker thread, the first if stmt will trigger a return because ReadyState() will be CLOSED.
The end of the story is: CloseInternal() is never called.

Consider to call CloseInternal instead ::Close() in this NewRunnableMethod.

@@ +401,2 @@
>  {
> +  AssertIsOnTargetThread();

Assert readyState == CLOSED.

@@ +401,4 @@
>  {
> +  AssertIsOnTargetThread();
> +  while (mMessagesToDispatch.GetSize() != 0) {
> +    delete static_cast<Message*>(mMessagesToDispatch.PopFront());

Maybe, in a separate patch, we can convert the Message to be UniquePtr.

@@ +410,5 @@
> +    ErrorResult rv;
> +    // run CleanupOnMainThread synchronously on main thread since it touches
> +    // observers and members only can be accessed on main thread.
> +    RefPtr<CleanupRunnable> runnable = new CleanupRunnable(this);
> +    runnable->Dispatch(rv);

1. I changed this method recently. You want to do: runnable->Dispatch(rv, Killing);

2. MOZ_ASSERT(!NS_FAILED(rv));

@@ +430,3 @@
>    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>    if (os) {
>      os->RemoveObserver(this, DOM_WINDOW_DESTROYED_TOPIC);

we should not have these observers if we are running in workers.

@@ +471,5 @@
> +    nsCOMPtr<nsIPrincipal> principal;
> +    if (window) {
> +      nsIDocument* doc = window->GetExtantDoc();
> +      if (!doc) {
> +        mRv.Throw(NS_ERROR_FAILURE);

you cannot do this. ErrorResult is not thread-safe. You should use nsresult instead.

@@ +490,5 @@
> +protected:
> +  // Raw pointer because this runnable is sync.
> +  EventSourceImpl* mImpl;
> +  const nsAString& mURL;
> +  ErrorResult& mRv;

this must be nsresult mRv;

Then make a getter method:

nsresult ErrorResult() const { return mRv; } and use it after the Dispatch();
aRv = runnable->ErrorResult();

@@ +526,5 @@
>  {
> +  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +  NS_ENSURE_STATE(os);
> +
> +  nsresult rv = os->AddObserver(this, DOM_WINDOW_DESTROYED_TOPIC, true);

This should not be done when EventSource runs on workers.

@@ +704,5 @@
> +    return;
> +  }
> +  int32_t srcCount, outCount;
> +  char16_t out[2];
> +  const char *p = aBuffer;

const char* p = aBuffer;
same for *end;

@@ +1174,5 @@
> +    rv = RestartConnection();
> +  } else {
> +    RefPtr<CallRestartConnection> runnable = new CallRestartConnection(this);
> +    ErrorResult result;
> +    runnable->Dispatch(result);

Terminating

@@ +1988,5 @@
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      return nullptr;
> +    }
> +    RefPtr<InitRunnable> runnable = new InitRunnable(eventSourceImp, aURL, aRv);
> +    runnable->Dispatch(aRv);

change this to: runnable->Dispatch(aRv, Terminating);

Comment 30

[Ming-Chou Shih [:stone]]

Attachment: Part1: Implement EventSource for Worker.

Followed the review comments to refine the patch. And add one more member variable mIsClosing to ensure only invoke CloseInternal once (since EventSource::Close might be called on main thread and worker thread)

Comment 31

[Ming-Chou Shih [:stone]]

Comment on attachment 8826074 [details] [diff] [review]
Part1: Implement EventSource for Worker.

Hi Baku,
Please kindly help to review if my patch is ok. Thanks,

Comment 32

[Andrea Marchesini [:baku]]

Comment on attachment 8826074 [details] [diff] [review]
Part1: Implement EventSource for Worker.

Review of attachment 8826074 [details] [diff] [review]:
-----------------------------------------------------------------

We need tests. and maybe write them as WPT. Thanks for doing this!

::: dom/base/EventSource.cpp
@@ +403,5 @@
>      return;
>    }
> +  mIsClosing = true;
> +  while (mMessagesToDispatch.GetSize() != 0) {
> +    delete static_cast<Message*>(mMessagesToDispatch.PopFront());

I really would like to see UniquePtr for this. Can you file a follow up?

@@ +421,5 @@
> +
> +  SetFrozen(false);
> +  ResetDecoder();
> +  mUnicodeDecoder = nullptr;
> +  // UpdateDontKeepAlive() can release the object. So hold a reference to this

why this comment? I guess you can just do:

mEventSource->UpdateDontKeepAlive();

there is nothing after this method. Why do you wan to use kungfuDeathGrip?

@@ +466,5 @@
> +    while (wp->GetParent()) {
> +      wp = wp->GetParent();
> +    }
> +    nsPIDOMWindowInner* window = wp->GetWindow();
> +    nsCOMPtr<nsIPrincipal> principal;

nsCOMPtr<nsIPrincipal> principal = window ? window->GetPrincipal() : wp->GetPrincipal();

@@ +747,5 @@
> +           mLastConvertionResult != NS_PARTIAL_MORE_INPUT &&
> +           mLastConvertionResult != NS_OK);
> +}
> +
> +class DataAvailableRunnable : public Runnable

final

@@ +794,2 @@
>                                      aCount, &totalRead);
> +  } else {

Follow up to remove this when necko supports the dispatching of DataAvailable on the target thread. File a bug.

@@ +797,5 @@
> +    // fallback to the main thread.
> +    AssertIsOnMainThread();
> +    auto data = MakeUniqueFallible<char[]>(aCount);
> +    if (!data) {
> +      return NS_ERROR_ABORT;

OUT_OF_MEMORY

@@ +1170,5 @@
> +EventSourceImpl::RestartConnection()
> +{
> +  AssertIsOnMainThread();
> +  nsresult rv = ResetConnection();
> +  if (NS_FAILED(rv)) {

NS_ENSURE_SUCCESS(rv, rv);

@@ +1174,5 @@
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("Failed to reset the connection!!!");
> +    return rv;
> +  }
> +  rv = SetReconnectionTimeout();

NS_ENSURE_SUCCESS(rv, rv);
return NS_OK;

@@ +1821,5 @@
> +    return true;
> +  }
> +
> +private:
> +  EventSourceImpl* mEventSourceImpl;

a comment here about the raw pointer.

@@ +1891,5 @@
> +{
> +  MOZ_ASSERT(IsClosed());
> +  MOZ_ASSERT(mWorkerPrivate);
> +  mWorkerPrivate->AssertIsOnWorkerThread();
> +  if (!mWorkerHolder) {

you don't need this if()

@@ +1944,5 @@
> +//-----------------------------------------------------------------------------
> +NS_IMETHODIMP
> +EventSourceImpl::CheckListenerChain()
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Should be on the main thread!");

MOZ_ASSERT remove any other NS_ASSERTION

@@ +2107,5 @@
> +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mImpl->mPrincipal)
> +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mImpl->mTimer)
> +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mImpl->mLoadGroup)
> +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mImpl->mHttpChannel)
> +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mImpl->mUnicodeDecoder)

read me next comment.

@@ +2115,5 @@
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(EventSource,
> +                                                DOMEventTargetHelper)
> +  if (tmp->mImpl) {
> +    NS_IMPL_CYCLE_COLLECTION_UNLINK(mImpl->mSrc)
> +    NS_IMPL_CYCLE_COLLECTION_UNLINK(mImpl->mPrincipal)

This is wrong. Traverse and unlink run on the target thread.
mPrincipal, mLoadGroup and others are main-thread only.
Here you should call a cleanup/close method.

::: dom/base/EventSource.h
@@ +46,5 @@
>    NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS_INHERITED(
>      EventSource, DOMEventTargetHelper)
>  
> +  // EventTarget
> +  virtual void DisconnectFromOwner() override

if this class is final, don't use virtual.
Everywhere in this class.

Comment 33

[Ming-Chou Shih [:stone]]

(In reply to Andrea Marchesini [:baku] from comment #32)
> > +  mIsClosing = true;
> > +  while (mMessagesToDispatch.GetSize() != 0) {
> > +    delete static_cast<Message*>(mMessagesToDispatch.PopFront());
> 
> I really would like to see UniquePtr for this. Can you file a follow up?
Created Bug 1330631 - Followup to bug 1267903 - Convert the EventSourceImpl::Message to be UniquePtr

> Follow up to remove this when necko supports the dispatching of
> DataAvailable on the target thread. File a bug.
Created Bug 1330632 - Followup to bug 1267903 - Remove DataAvailableRunnable when necko supports the dispatching of DataAvailable on the target thread

And many thanks for the review.

Comment 34

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef38ff47e306596b6f2f2d6888596e4d19a49a48

Comment 35

[Ming-Chou Shih [:stone]]

Attachment: Part1: Implement EventSource for Worker.

Refined the patch and changed its summary

Comment 36

[Ming-Chou Shih [:stone]]

Attachment: Part2: Enable related test cases.

Updated the patch summary

Comment 37

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63b2679e3af4bcef25e689a04a99ae0c2b98a4d4

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eae4cf8e37a8
Part 1: Implement EventSource for Worker. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c7c25bb30c
Part 2: Enable related test cases. r=baku

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eae4cf8e37a8
https://hg.mozilla.org/mozilla-central/rev/e8c7c25bb30c

Comment 40

[Boris Zbarsky [:bzbarsky]]

Please send an intent to ship for this.  Ideally that should have happened before we landed it, but better late than never.  ;)

Comment 41

[Ming-Chou Shih [:stone]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #40)
> Please send an intent to ship for this.  Ideally that should have happened
> before we landed it, but better late than never.  ;)
Submitted https://groups.google.com/forum/#!topic/mozilla.dev.platform/UEEnz_2-Oos. and many thanks for reminding me to do it.

Comment 42

[Chris Mills [:cmills]]

I've documented this (which also required writing most of the EventSource pages - the docs were rather incomplete!)

I've updated the browser support information in relevant existing pages:

https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events
https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events/Using_server-sent_events
https://developer.mozilla.org/en-US/docs/Web/API/EventSource
https://developer.mozilla.org/en-US/docs/Web/API/EventSource/EventSource
https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Functions_and_classes_available_to_workers

I've also created a bunch of new pages:

https://developer.mozilla.org/en-US/docs/Web/API/EventSource/readyState
https://developer.mozilla.org/en-US/docs/Web/API/EventSource/url
https://developer.mozilla.org/en-US/docs/Web/API/EventSource/withCredentials
https://developer.mozilla.org/en-US/docs/Web/API/EventSource/onerror
https://developer.mozilla.org/en-US/docs/Web/API/EventSource/onmessage
https://developer.mozilla.org/en-US/docs/Web/API/EventSource/onopen
https://developer.mozilla.org/en-US/docs/Web/API/EventSource/close

Last, I've added a note to the Fx53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53#DOM_HTML_DOM 

Let me know if this all looks OK. thanks!

Comment 43

[Hamish Willee]

This adds support for dedicated and shared workers only - not service workers.

1. Is this intentional - ie is there a reason not to support service workers?
2. Assuming it is unintentional, have we since added support for service workers?

Context is that I want to update this doc to state whether service workers are generally not supported or just not supported by FF.

Comment 44

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Hamish Willee from comment #43)

This adds support for dedicated and shared workers only - not service workers.

1. Is this intentional - ie is there a reason not to support service workers?
2. Assuming it is unintentional, have we since added support for service workers?

This was intentionally not exposed on ServiceWorkers at the time but now should be. I filed bug 1681218 on exposing EventSource on ServiceWorkers. More details can be found at https://github.com/w3c/ServiceWorker/issues/947 with https://github.com/w3c/ServiceWorker/issues/947#issuecomment-411341963 being :annevk's conclusion that it was too late to unship.

Comment 45

[Andrew Sutherland [:asuth] (he/him)]

BTW, I have a tool at https://worker-playground.glitch.me/ derived from a tool :smaug created that lets you type, for example, EventSource in a text box and see what it evaluates to in Dedicated Workers, Shared Workers, and Service Workers which can be useful. (And I mention it because I used it and it's not very discoverable.)

Comment 46

[Hamish Willee]

Thanks Andrew! I've fixed up the doc note to point to your bug post.

I'm also working on the higher level docs on what functions you can call in functions and classes. I'll ensure that test tool site gets referenced!