Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: smaug, Assigned: stone)

Tracking

({dev-doc-complete})

36 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [tw-dom])

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

3 years ago
Looks like at least some browsers support it in workers, and per spec we should too.
Is this more urgent than backlog?
Flags: needinfo?(bugs)
Whiteboard: btpp-backlog
(Reporter)

Comment 2

3 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.
Flags: needinfo?(bugs)
(Reporter)

Comment 3

3 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 ;) )
Whiteboard: btpp-backlog → btpp-backlog [tw-dom]
(Reporter)

Comment 4

3 years ago
Someone just sent email to public-webapps@w3.org complaining that Chrome supports EventSource in ServiceWorkers (and Workers) but Firefox doesn't.
Is Bug 876498 a duplicate of this? 

Set to P2 as I am working on making it planned.
Priority: -- → P2
Whiteboard: btpp-backlog [tw-dom] → [tw-dom]
Duplicate of this bug: 876498
Stone is willing to learn more about Workers :)
Assignee: nobody → sshih
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.
(Reporter)

Comment 10

3 years ago
Well, I'd say we should expose. If WebSocket is there, EventSource should too.
(IMO, also XHR, but others may disagree.)
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.
(Reporter)

Comment 12

3 years ago
I would expect waitUntil to be used here. Just the same thing what you mentioned would be used for sub-workers, no?
(Reporter)

Comment 13

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

3 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.  :-(
(Assignee)

Comment 16

3 years ago
Attachment #8814345 - Flags: feedback?(amarchesini)
(Assignee)

Comment 17

3 years ago
Attachment #8814346 - Flags: feedback?(amarchesini)
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.
Attachment #8814345 - Flags: feedback?(amarchesini) → feedback+
Attachment #8814346 - Flags: feedback?(amarchesini) → feedback+
(Assignee)

Comment 19

3 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.
Flags: needinfo?(amarchesini)
I filed bug 1320744 for having HttpChannelChild implementing nsIThreadRetargetableRequest interface.
Depends on: 1320744
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 :)
(Assignee)

Comment 22

3 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.
> > 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.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 24

2 years ago
Followed feedbacks to refine the patch
Attachment #8814345 - Attachment is obsolete: true
(Assignee)

Comment 25

2 years ago
Posted patch Enable related test cases (obsolete) — Splinter Review
Followed feedbacks to refine the patch
Attachment #8814346 - Attachment is obsolete: true
(Assignee)

Comment 26

2 years ago
Attachment #8821946 - Attachment is obsolete: true
(Assignee)

Comment 27

2 years ago
Attachment #8821947 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8821948 - Flags: review?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8821949 - Flags: review?(amarchesini)
(Assignee)

Comment 28

2 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.
Attachment #8821949 - Flags: review?(amarchesini) → review+
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);
Attachment #8821948 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 30

2 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)
Attachment #8821948 - Attachment is obsolete: true
(Assignee)

Comment 31

2 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,
Attachment #8826074 - Flags: review?(amarchesini)
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.
Attachment #8826074 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

2 years ago
Blocks: 1330631
(Assignee)

Updated

2 years ago
Blocks: 1330632
(Assignee)

Comment 33

2 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.
(Assignee)

Comment 35

2 years ago
Refined the patch and changed its summary
Attachment #8826074 - Attachment is obsolete: true
Attachment #8827019 - Flags: review+
(Assignee)

Comment 36

2 years ago
Updated the patch summary
Attachment #8821949 - Attachment is obsolete: true
Attachment #8827020 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 38

2 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
Keywords: checkin-needed

Comment 39

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eae4cf8e37a8
https://hg.mozilla.org/mozilla-central/rev/e8c7c25bb30c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please send an intent to ship for this.  Ideally that should have happened before we landed it, but better late than never.  ;)
Flags: needinfo?(sshih)
(Assignee)

Comment 41

2 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.
Flags: needinfo?(sshih)

Updated

a year ago
Depends on: 1436544
You need to log in before you can comment on or make changes to this bug.