Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Description • 8 years ago

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

	Andrew Overholt [:overholt]
	Comment 1 • 8 years ago

Is this more urgent than backlog?

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 2 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 3 • 8 years ago

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 ;) )

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 4 • 8 years ago

Someone just sent email to public-webapps@w3.org complaining that Chrome supports EventSource in ServiceWorkers (and Workers) but Firefox doesn't.

	Sam Ruby
	Comment 5 • 8 years ago

That would be me.

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 6 • 8 years ago

Is Bug 876498 a duplicate of this? 

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 8 • 8 years ago

Stone is willing to learn more about Workers :)

	Ben Kelly [:bkelly, not reviewing]
	Comment 9 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 10 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 11 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 12 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 13 • 8 years ago

(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?)

	Ben Kelly [:bkelly, not reviewing]
	Comment 14 • 8 years ago

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.

	Sam Ruby
	Comment 15 • 8 years ago

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.  :-(

	Andrea Marchesini [:baku]
	Comment 18 • 8 years ago

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.

	Ming-Chou Shih [:stone] Assignee
	Comment 19 • 8 years ago

(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.

	Andrea Marchesini [:baku]
	Comment 20 • 8 years ago

I filed bug 1320744 for having HttpChannelChild implementing nsIThreadRetargetableRequest interface.

	Andrea Marchesini [:baku]
	Comment 21 • 8 years ago

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 :)

	Ming-Chou Shih [:stone] Assignee
	Comment 22 • 8 years ago

(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.

	Andrea Marchesini [:baku]
	Comment 23 • 7 years ago

> > 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.

	Ming-Chou Shih [:stone] Assignee
	Comment 24 • 7 years ago

Followed feedbacks to refine the patch

	Ming-Chou Shih [:stone] Assignee
	Comment 25 • 7 years ago

Followed feedbacks to refine the patch

	Ming-Chou Shih [:stone] Assignee
	Comment 28 • 7 years ago

(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.

	Andrea Marchesini [:baku]
	Comment 29 • 7 years ago

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);

	Ming-Chou Shih [:stone] Assignee
	Comment 30 • 7 years ago

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)

	Ming-Chou Shih [:stone] Assignee
	Comment 31 • 7 years ago

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,

	Andrea Marchesini [:baku]
	Comment 32 • 7 years ago

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.

	Ming-Chou Shih [:stone] Assignee
	Comment 33 • 7 years ago

(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.

	Ming-Chou Shih [:stone] Assignee
	Comment 34 • 7 years ago

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

	Ming-Chou Shih [:stone] Assignee
	Comment 35 • 7 years ago

Refined the patch and changed its summary

	Ming-Chou Shih [:stone] Assignee
	Comment 36 • 7 years ago

Updated the patch summary

	Ming-Chou Shih [:stone] Assignee
	Comment 37 • 7 years ago

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

	Pulsebot
	Comment 38 • 7 years ago

Pushed by ryanvm@gmail.com:
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

	Phil Ringnalda (:philor)
	Comment 39 • 7 years ago
bugherder

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

	Boris Zbarsky [:bzbarsky]
	Comment 40 • 7 years ago

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

	Ming-Chou Shih [:stone] Assignee
	Comment 41 • 7 years ago

(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.

	Chris Mills [:cmills]
	Comment 42 • 7 years ago

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!