1308956 - Get rid of MessagePortList

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

There is no such thing in the spec now.  It uses a FrozenArray instead.

We presumably want to use an nsTArray for internal storage, have an internal accessor for it, and then do the usual [Constant, Cached, Frozen] sequence thing (or Pure if the list can change, but I expect it can't for a MessageEvent).

Note also https://github.com/whatwg/html/issues/1882 about whether .ports should ever be null.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - MessagePortList removed from events

I don't remove MessagePortList yet because it's used by nsFrameMessageManager::ReceiveMessage.
I'll remove it in a separate patch.

Comment 2

[Boris Zbarsky [:bzbarsky]]

>   if (aParam.mPorts.WasPassed() && !aParam.mPorts.Value().IsNull()) {

You shouldn't need the WasPassed() thing.  Just make the default value null in the dictionary, no?

But past that, treating empty list as an alias for "null" isn't right; in terms of the current spec definition they should lead to different observable behavior...

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - files removed

Comment 4

[Olli Pettay [:smaug]]

Comment on attachment 8799469 [details] [diff] [review]
part 1 - MessagePortList removed from events

>@@ -135,22 +134,17 @@ MessageEvent::Constructor(EventTarget* a
>     } else {
>       event->mPortSource = aParam.mSource.Value().GetAsMessagePort();
>     }
> 
>     MOZ_ASSERT(event->mWindowSource || event->mPortSource);
>   }
> 
>   if (aParam.mPorts.WasPassed() && !aParam.mPorts.Value().IsNull()) {
Yeah, as bz said, we should fix this stuff to follow the spec.
Looks like our MessageEventInit isn't right atm.
Could you upload another patch here to fix MessageEventInit and fix this ctor handling, or perhaps just fix it in a new version of 
this patch.


+MessageEvent::GetPorts(Nullable<nsTArray<RefPtr<MessagePort>>>& aPorts)
 {
-  MOZ_ASSERT(!mPorts && aPorts);
-  mPorts = aPorts;
+  if (mPorts.IsEmpty()) {
+    aPorts.SetNull();
+    return;
+  }
This is not what the spec says should happen. Null should be return only if the event was initialized with null ports.  empty != null

>+++ b/dom/webidl/MessageEvent.webidl
>@@ -35,17 +35,18 @@ interface MessageEvent : Event {
>    */
>   readonly attribute (WindowProxy or MessagePort)? source;
> 
>   /**
>    * Initializes this event with the given data, in a manner analogous to
>    * the similarly-named method on the nsIDOMEvent interface, also setting the
>    * data, origin, source, and lastEventId attributes of this appropriately.
>    */
>-  readonly attribute MessagePortList? ports;
>+  [Constant, Cached, Frozen]
>+  readonly attribute sequence<MessagePort>? ports;
The annotations look wrong, given the C++ implementation. One may call initMessageEvent and that should re-set the
.ports value. 
I'm not seeing ClearCachedPortsValue anywhere in C++ 

>+++ b/dom/webidl/ServiceWorkerMessageEvent.webidl
>@@ -30,17 +30,18 @@ interface ServiceWorkerMessageEvent : Ev
> 
>   /**
>    * The service worker or port which originated this event.
>    * FIXME: Use SameOject after IDL spec is updated
>    * https://bugzilla.mozilla.org/show_bug.cgi?id=1196097
>    */
>   [Constant] readonly attribute (ServiceWorker or MessagePort)? source;
> 
>-  [SameObject] readonly attribute MessagePortList? ports;
>+  [Constant, Cached, Frozen]
>+  readonly attribute sequence<MessagePort>? ports;
Same here

> #include "mozilla/dom/WorkerScope.h"
>@@ -1241,39 +1240,38 @@ ExtendableMessageEvent::Constructor(mozi
>       event->mServiceWorker = aOptions.mSource.Value().Value().GetAsServiceWorker();
>     } else if (aOptions.mSource.Value().Value().IsMessagePort()){
>       event->mMessagePort = aOptions.mSource.Value().Value().GetAsMessagePort();
>     }
>     MOZ_ASSERT(event->mClient || event->mServiceWorker || event->mMessagePort);
>   }
> 
>   if (aOptions.mPorts.WasPassed() && !aOptions.mPorts.Value().IsNull()) {
This is similar to MessageEvent case, expect that the spec is also buggy here.

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

>+ExtendableMessageEvent::GetPorts(Nullable<nsTArray<RefPtr<MessagePort>>>& aPorts)
> {
>-  return mPorts;
>+  if (mPorts.IsEmpty()) {
>+    aPorts.SetNull();
>+    return;
>+  }
>+
>+  aPorts.SetValue(mPorts);
Same issue as that the GetPorts in MessageEvent


>+ServiceWorkerMessageEvent::GetPorts(Nullable<nsTArray<RefPtr<MessagePort>>>& aPorts)
> {
>-  MOZ_ASSERT(!mPorts && aPorts);
>-  mPorts = aPorts;
>+  if (mPorts.IsEmpty()) {
>+    aPorts.SetNull();
>+    return;
>+  }
>+
>+  aPorts.SetValue(mPorts);
Here too

Comment 5

[Boris Zbarsky [:bzbarsky]]

Spec just got updated to make .ports not nullable anymore.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 1 - MessagePortList removed from events

WebIDL updated

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 1 - MessagePortList removed from events

I had to touch the codegen because it doesn't include XrayWrapper.h for ExtendedMessageEvent. This seems wrong to me and it hides another bug. I need feedback for that.

Comment 8

[Olli Pettay [:smaug]]

Filed https://github.com/w3c/ServiceWorker/issues/989

Comment 9

[Olli Pettay [:smaug]]

Comment on attachment 8799628 [details] [diff] [review]
part 1 - MessagePortList removed from events

I wonder if we should wait a day or two, given that the specs 
(HTML and ServiceWorker) are very much in flux related to this stuff.

> 
> dictionary MessageEventInit : EventInit {
>   any data;
>   DOMString origin;
>   DOMString lastEventId;
>   (Window or MessagePort)? source = null;
>-  sequence<MessagePort>? ports;
>+  sequence<MessagePort> ports = [];
> };
So the dictionary isn't still following the spec. Want to fix that in this bug or file a followup?
All the properties have default values in the spec.


>+void
>+ExtendableMessageEvent::GetPorts(nsTArray<RefPtr<MessagePort>>& aPorts)
> {
>-  return mPorts;
>+  aPorts.Clear();
>+  aPorts.AppendElements(mPorts);
> }
So this is against the current spec...

Ok, I think given the current mess in the specs, we shouldn't do anything here quite yet
https://github.com/whatwg/html/issues/1882#issuecomment-252900662

Comment 10

[Olli Pettay [:smaug]]

Comment on attachment 8799661 [details] [diff] [review]
part 1 - MessagePortList removed from events

Hmm, what is this? wrong patch or another version of the previous one?

Comment 11

[Andrea Marchesini [:baku]]

What about if we just do what this bug is about?
Then in a follow up we can update interfaces.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8799628 [details] [diff] [review]
part 1 - MessagePortList removed from events

>+  aPorts.Clear();
>+  aPorts.AppendElements(mPorts);

  aPorts = mPorts;

should do the trick.  And in the other similar places too.

> dictionary ExtendableMessageEventInit : ExtendableEventInit {

Shouldn't this change to have a non-nullable ports if the property on ExtendableMessageEvent is non-nullable?

> dictionary ServiceWorkerMessageEventInit : EventInit

And here.

Comment 13

[Olli Pettay [:smaug]]

Well, ExtendableMessageEventInit and ServiceWorkerMessageEventInit have still nullable ports in the spec. Which is why I'd like to see the specs sorted out. Maybe we'll end up having nullable ports everywhere. Or non-nullable. Who knows.

Comment 14

[Olli Pettay [:smaug]]

(In reply to Andrea Marchesini [:baku] from comment #11)
> What about if we just do what this bug is about?
> Then in a follow up we can update interfaces.

That is fine. Then don't change the behavior otherwise, just replace use of MessagePortList with a sequence.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 1 - MessagePortList removed from events

Comment 16

[Olli Pettay [:smaug]]

Comment on attachment 8800378 [details] [diff] [review]
part 1 - MessagePortList removed from events

>+++ b/dom/webidl/MessageEvent.webidl
>@@ -35,17 +35,18 @@ interface MessageEvent : Event {
>    */
>   readonly attribute (WindowProxy or MessagePort)? source;
> 
>   /**
>    * Initializes this event with the given data, in a manner analogous to
>    * the similarly-named method on the nsIDOMEvent interface, also setting the
>    * data, origin, source, and lastEventId attributes of this appropriately.
>    */
>-  readonly attribute MessagePortList? ports;
>+  [Constant, Cached, Frozen]
>+  readonly attribute sequence<MessagePort>? ports;
The missing ClearCachedPortsValue crept in again to this patch.
We really need a test for it, some very simple case where one first creates event with some
ports and then re-initializes with different ports, and .ports should give the new ports.

>-MessagePortList*
>-ExtendableMessageEvent::GetPorts() const
>+void
>+ExtendableMessageEvent::GetPorts(Nullable<nsTArray<RefPtr<MessagePort>>>& aPorts)
> {
>-  return mPorts;
>+  if (mPorts.IsEmpty()) {
>+    aPorts.SetNull();
>+    return;
>+  }
Empty ports doesn't mean null.

>+ServiceWorkerMessageEvent::GetPorts(Nullable<nsTArray<RefPtr<MessagePort>>>& aPorts)
> {
>-  MOZ_ASSERT(!mPorts && aPorts);
>-  mPorts = aPorts;
>+  if (mPorts.IsEmpty()) {
>+    aPorts.SetNull();
>+    return;
>+  }
ditto.


Sounds like we're missing test for those events when null is passed as ports.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 1 - MessagePortList removed from events

Comment 18

[Olli Pettay [:smaug]]

Comment on attachment 8800565 [details] [diff] [review]
part 1 - MessagePortList removed from events

>   if (!subworker) {
>     info("Create a subworkers. ID: " + evt.data);
>     subworker = new Worker('worker_messageChannel.js');
>     subworker.onmessage = function(e) {
>       info("Proxy a message to the parent.");
>-      postMessage(e.data, e.ports);
>+      postMessage(e.data, e.ports ? e.ports : []);
Why is this change needed.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d090c04d8f
Get rid of MessagePortList - part 1 - MessagePortList removed from events, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/36d6e588e7d4
Get rid of MessagePortList - part 2 - files removed, r=smaug

Comment 20

[Andrea Marchesini [:baku]]

> Why is this change needed.

It's not needed. I reverted that change before landing the patches.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9d090c04d8f
https://hg.mozilla.org/mozilla-central/rev/36d6e588e7d4