All users were logged out of Bugzilla on October 13th, 2018

Get rid of MessagePortList

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: baku, Mentored)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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

Updated

2 years ago
Mentor: bzbarsky
(Assignee)

Comment 1

2 years ago
Created attachment 8799469 [details] [diff] [review]
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.
Assignee: nobody → amarchesini
Attachment #8799469 - Flags: review?(bugs)
(Reporter)

Comment 2

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

Comment 3

2 years ago
Created attachment 8799472 [details] [diff] [review]
part 2 - files removed
Attachment #8799472 - Flags: review?(bugs)

Comment 4

2 years ago
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
Attachment #8799469 - Flags: review?(bugs) → review-

Updated

2 years ago
Attachment #8799472 - Flags: review?(bugs) → review+
(Reporter)

Comment 5

2 years ago
Spec just got updated to make .ports not nullable anymore.
(Assignee)

Comment 6

2 years ago
Created attachment 8799628 [details] [diff] [review]
part 1 - MessagePortList removed from events

WebIDL updated
Attachment #8799469 - Attachment is obsolete: true
Attachment #8799628 - Flags: review?(bugs)
(Assignee)

Comment 7

2 years ago
Created attachment 8799661 [details] [diff] [review]
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.
Attachment #8799661 - Flags: review?(bugs)

Comment 9

2 years ago
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
Attachment #8799628 - Flags: review?(bugs)
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?
Attachment #8799661 - Flags: review?(bugs)
(Assignee)

Comment 11

2 years ago
What about if we just do what this bug is about?
Then in a follow up we can update interfaces.
Flags: needinfo?(bugs)
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.
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.
Flags: needinfo?(bugs)
(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.
(Assignee)

Comment 15

2 years ago
Created attachment 8800378 [details] [diff] [review]
part 1 - MessagePortList removed from events
Attachment #8799628 - Attachment is obsolete: true
Attachment #8799661 - Attachment is obsolete: true
Attachment #8800378 - Flags: review?(bugs)
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.
Attachment #8800378 - Flags: review?(bugs) → review-
(Assignee)

Comment 17

2 years ago
Created attachment 8800565 [details] [diff] [review]
part 1 - MessagePortList removed from events
Attachment #8800378 - Attachment is obsolete: true
Attachment #8800565 - Flags: review?(bugs)
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.
Attachment #8800565 - Flags: review?(bugs) → review+

Comment 19

2 years ago
Pushed by amarchesini@mozilla.com:
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
(Assignee)

Comment 20

2 years ago
> Why is this change needed.

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

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f9d090c04d8f
https://hg.mozilla.org/mozilla-central/rev/36d6e588e7d4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.