1330631 - Followup to bug 1267903 - Convert the EventSourceImpl::Message to be UniquePtr

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[Ming-Chou Shih [:stone]]

Attachment: Convert the EventSourceImpl::Message to be UniquePtr

Comment 2

[Ming-Chou Shih [:stone]]

Comment on attachment 8828236 [details] [diff] [review]
Convert the EventSourceImpl::Message to be UniquePtr

This is a follow-up of bug 1267903 to convert the EventSourceImpl::Message to be UniquePtr. I'd request your review if it's ok. Thanks.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8828236 [details] [diff] [review]
Convert the EventSourceImpl::Message to be UniquePtr

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

You are changing the logic. I would like to know if this is following the spec. Please check.

::: dom/base/EventSource.cpp
@@ +111,5 @@
>    void ResetDecoder();
>    nsresult SetReconnectionTimeout();
>  
>    void AnnounceConnection();
> +  void DispatchAllMessageEvents(UniquePtr<MessageArray>&& aMessageArray);

1. If it receives an array of events, we should probably call it: DispatchMessageEvents()
2. what about if you use Move?
3. or maybe just pass an array and call the method: StealAndDispatchMessageEvents, and then you do: SwapElements()

@@ +1388,1 @@
>    *message = mCurrentMessage;

I wonder if we can get rid of mCurrentMessage somehow. Or maybe, just make it a UniquePtr so that here, we don't need to copy data.

@@ +1389,1 @@
>    ClearFields();

If we do that, we can get rid of this.

@@ +1589,5 @@
>  
>    if (IsClosed()) {
>      return NS_ERROR_ABORT;
>    }
> +  UniquePtr<MessageArray> messageArray = MakeUnique<MessageArray>();

I don't like this too much. What about if you just do:

1. MessageArray messageArray;
2. then you pass it, as a reference to AppendCurrentMessage()
3. You call: StealAndDispatchMessageEvents(messageArray);

@@ +1732,5 @@
>  
> +  nsCOMPtr<nsIRunnable> event = NewRunnableMethod<UniquePtr<MessageArray>&&>
> +    (this, &EventSourceImpl::DispatchAllMessageEvents, Move(messageArray));
> +  NS_ENSURE_STATE(event);
> +  return Dispatch(event.forget(), NS_DISPATCH_NORMAL);

Here you are changing the logic. Before we had 1 runnable. It was dispatched when there was 1 message and if more were coming, when the runnable was executed, everything was dispatched.

Here you are dispatching 1 message at the time.
So, why do you need the list? Can you just do: DispatchCurrentMessageEvent() ?

What do the spec say about it?

Comment 4

[Ming-Chou Shih [:stone]]

Attachment: Convert the EventSourceImpl::Message to be UniquePtr

Comment 5

[Ming-Chou Shih [:stone]]

Attachment: Convert the EventSourceImpl::Message to be UniquePtr

Comment 6

[Ming-Chou Shih [:stone]]

Attachment: Part1: Convert the EventSourceImpl::Message to be UniquePtr

(In reply to Andrea Marchesini [:baku] from comment #3)
> I wonder if we can get rid of mCurrentMessage somehow. Or maybe, just make
> it a UniquePtr so that here, we don't need to copy data.
I think we can't get rid of mCurrentMessage because the message may be transmitted by multiple chunks. So make it a UniquePtr to avoid copy data

> >    ClearFields();
> If we do that, we can get rid of this.
We can get rid of resetting mCurrentMessage but still need to clear incomplete mLastFieldName and mLastFieldValue.

> Here you are changing the logic. Before we had 1 runnable. It was dispatched
> when there was 1 message and if more were coming, when the runnable was
> executed, everything was dispatched.
> 
> Here you are dispatching 1 message at the time.
> So, why do you need the list? Can you just do: DispatchCurrentMessageEvent()
> ?
> 
> What do the spec say about it?
Spec [1] only says we have to queue a task which, dispatch the message if the ready state isn't closed. Looks like current implementation is a kind of optimization to prevent adding a task to queue for each message?

BTW, I found moving all messages to be dispatched to runnable breaks the original logic, which queue messages when window freezes and dispatch them when window thaws. We still need a list in EventSourceImpl to keep the messages.

[1] https://html.spec.whatwg.org/multipage/comms.html#dispatchMessage

Comment 7

[Ming-Chou Shih [:stone]]

Attachment: Part2: Refine EventSourceImpl::DispatchAllMessageEvents

I found there are some problems when DispatchAllMessageEvents is run after window freezes. Create part2 to refine it. Or maybe I should create another bug for it?

Comment 8

[Ming-Chou Shih [:stone]]

Comment on attachment 8833133 [details] [diff] [review]
Part1: Convert the EventSourceImpl::Message to be UniquePtr

Hi Baku,
I had refined my patch and would like to ask for your review. Thanks.

Comment 9

[Ming-Chou Shih [:stone]]

Comment on attachment 8833135 [details] [diff] [review]
Part2: Refine EventSourceImpl::DispatchAllMessageEvents

Hi Baku,
I had refined my patch and would like to ask for your review. Thanks.

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8833133 [details] [diff] [review]
Part1: Convert the EventSourceImpl::Message to be UniquePtr

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

Just some additional details

::: dom/base/EventSource.cpp
@@ +1418,5 @@
>    if (message->mLastEventID.IsEmpty() && !mLastEventID.IsEmpty()) {
>      message->mLastEventID.Assign(mLastEventID);
>    }
>  
> +  mMessagesToDispatch.AppendElement(Move(message));

Make this array FallibleTArray and convert this as:

if (!mMessageToDispatch.AppendElement(Move(message), fallible))) {
  return NS_ERROR_OUT_OF_MEMORY;
}

@@ +1461,5 @@
>        return;
>      }
>    }
>    JSContext* cx = jsapi.cx();
> +  for (uint32_t i = 0; i < mMessagesToDispatch.Length(); ++i) {

Here you are changing the logic.
When you dispatch an event, you are executing JS code. This means that something can spin the event loop.

Do something like:

while (!mMessageToDispatch.IsEmpty()) {
  UniquePtr<Message> message(Move(mMessageToDispatch.ElementAt(0)));
  mMessageToDispatch.RemoveAt(0);

  ...

@@ +1495,5 @@
>      }
>  
>      mLastEventID.Assign(message->mLastEventID);
>      if (IsClosed() || IsFrozen()) {
> +      break;

Here you are breaking the logic again, because maybe this is not the last message, and doing a break, you are also executing mMessagesToDispatch.Clear(), and you are removing all the pending messages after the current one.

@@ +1507,5 @@
>  {
>    AssertIsOnTargetThread();
>    // mLastEventID and mReconnectionTime must be cached
> +  if (mCurrentMessage) {
> +    mCurrentMessage = nullptr;

remove if()... just do: mCurrentMessage = nullptr;

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8833135 [details] [diff] [review]
Part2: Refine EventSourceImpl::DispatchAllMessageEvents

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

File a new bug for this. Thanks!

::: dom/base/EventSource.cpp
@@ +1493,5 @@
>        return;
>      }
>  
>      mLastEventID.Assign(message->mLastEventID);
> +    if (IsClosed()) {

Why do you remove IsFrozen() ?

Comment 12

[Ming-Chou Shih [:stone]]

(In reply to Andrea Marchesini [:baku] from comment #11)
> >      mLastEventID.Assign(message->mLastEventID);
> > +    if (IsClosed()) {
> 
> Why do you remove IsFrozen() ?
Since the window is frozen on the main thread as well as dispatching messages, I think it's redundant because we had checked it before entering the loop.

Comment 13

[Ming-Chou Shih [:stone]]

(In reply to Ming-Chou Shih [:stone] from comment #12)
> (In reply to Andrea Marchesini [:baku] from comment #11)
> > >      mLastEventID.Assign(message->mLastEventID);
> > > +    if (IsClosed()) {
> > 
> > Why do you remove IsFrozen() ?
> Since the window is frozen on the main thread as well as dispatching
> messages, I think it's redundant because we had checked it before entering
> the loop.
I'm wrong. Dispatch message could spin the event loop and change the window to freeze. Refined it and attached to new created bug 1337619

Comment 14

[Ming-Chou Shih [:stone]]

Attachment: Convert the EventSourceImpl::Message to be UniquePtr

Change the type of mMessagesToDispatch back to nsDeque since we have to process the message one by one and FIFO.

Comment 15

[Ming-Chou Shih [:stone]]

Attachment: Convert the EventSourceImpl::Message to be UniquePtr

Comment 16

[Ming-Chou Shih [:stone]]

Attachment: Convert the EventSourceImpl::Message to be UniquePtr

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8834732 [details] [diff] [review]
Convert the EventSourceImpl::Message to be UniquePtr

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

::: dom/base/EventSource.cpp
@@ +1511,4 @@
>  EventSourceImpl::ClearFields()
>  {
>    AssertIsOnTargetThread();
>    // mLastEventID and mReconnectionTime must be cached

remove this comment.

Comment 18

[Ming-Chou Shih [:stone]]

Attachment: Convert the EventSourceImpl::Message to be UniquePtr

Updated the patch summary

Comment 19

[Ming-Chou Shih [:stone]]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e69dcd6ee700
Convert the EventSourceImpl::Message to be UniquePtr. r=baku

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e69dcd6ee700