1143717 - Implement the ServiceWorkerMessageEvent interface

Status: RESOLVED FIXED

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

Description

[Cătălin Badea (:catalinb)]

The webidl for MessageEvent needs to be updated to allow ServiceWorker and Client sources.

This bug tracks that we are up-to-date with the changes at https://www.w3.org/Bugs/Public/show_bug.cgi?id=28199

Comment 1

[Cătălin Badea (:catalinb)]

Service Workers now use ServiceWorkerMessageEvent and ExtendableMessageEvent. We should revert the webidl changes on MessageEvent.source made in 1142015 and use these new interfaces instead.

https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#serviceworkermessage-event-interface
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#extendablemessage-event-interface

Comment 2

[Dimi Lee [ooo 16.04 - 21.04］]

I would like to work on this bug, so the purpose of this bug is to implement serviceworkermessageevent & extendablemessageevent ?

Comment 3

[Dimi Lee [ooo 16.04 - 21.04］]

Hi,
I am now working on |serviceworkermessageevent| according to https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#serviceworkermessage-event-interface, should I use this bug or create a new bug ?

Comment 4

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: (WIP)Patch v1

Comment 5

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: (WIP)Patch v2

Comment 6

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8629317 [details] [diff] [review]
(WIP)Patch v2

Hi Catalin,
I wrote a WIP patch for serviceworkermessageevent, could you help take a look at it to make sure i am on the right track ? 

And I am not sure if there is any other place I need to create |serviceworkermessageevent|, now I only add it in ServiceWorkerClient.cpp
Also not quite sure if I should set |source| and |ports|.

Thanks for help!

Comment 7

[Cătălin Badea (:catalinb)]

(In reply to Dimi Lee[:dimi][:dlee] from comment #3)
> Hi,
> I am now working on |serviceworkermessageevent| according to
> https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.
> html#serviceworkermessage-event-interface, should I use this bug or create a
> new bug ?

Yes, using this bug is fine.

Comment 8

[Cătălin Badea (:catalinb)]

Comment on attachment 8629317 [details] [diff] [review]
(WIP)Patch v2

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

This is on the right track. Be sure to also to remove the ServiceWorker occurrences from MessageEvent since they are no longer needed.

::: dom/webidl/ServiceWorkerMessageEvent.webidl
@@ +25,5 @@
> +  readonly attribute DOMString origin;
> +
> +  /**
> +   * The ServiceWorker object whose associated service worker the message
> +   * is sent from.

This comment is wrong.

@@ +30,5 @@
> +   */
> +  readonly attribute DOMString lastEventId;
> +
> +  //In Spec : [SameObject] readonly attribute (ServiceWorker or MessagePort)? source;
> +  readonly attribute (ServiceWorker or MessagePort)? source;

I think [SameObject] should be allowed for union types. You should check this with bz.

@@ +40,5 @@
> +{
> +  any data;
> +  DOMString origin;
> +  DOMString lastEventId;
> +  (ServiceWorker or MessagePort)? source = null;

The spec doesn't include a default null value.

::: dom/workers/ServiceWorkerClient.cpp
@@ +131,5 @@
>      }
>  
> +    RootedDictionary<ServiceWorkerMessageEventInit> init(aCx);
> +
> +    //init.mSource.SetValue().SetAsServiceWorker() = /* serviceworker??? */

You can either use the init object or add a helper function: InitEvent(..).

::: dom/workers/ServiceWorkerMessageEvent.cpp
@@ +19,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(ServiceWorkerMessageEvent)
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(ServiceWorkerMessageEvent, Event)
> +  tmp->mData.setUndefined();

NS_IMPL_CYCLE_COLLECTION_UNLINK(mServiceWorker)
NS_IMPL_CYCLE_COLLECTION_UNLINK(mMessagePort)
NS_IMPL_CYCLE_COLLECTION_UNLINK(mPorts)

@@ +22,5 @@
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(ServiceWorkerMessageEvent, Event)
> +  tmp->mData.setUndefined();
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(ServiceWorkerMessageEvent, Event)

NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mServiceWorker) etc.

@@ +30,5 @@
> +  NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mData)
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END
> +
> +//NS_IMPL_ADDREF_INHERITED(ServiceWorkerMessageEvent, Event)
> +//NS_IMPL_RELEASE_INHERITED(ServiceWorkerMessageEvent, Event)

you should keep these two lines. Overriding the addref/release functions helps in displaying the correct type when leaks occur.

Use this for the QueryInterface definition:
NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(..)
NS_INTERFACE_MAP_END_INHERITING(..)

@@ +56,5 @@
> +
> +
> +void
> +ServiceWorkerMessageEvent::GetData(JSContext* aCx, JS::MutableHandle<JS::Value> aData,
> +                                   ErrorResult& aRv)

const

@@ +66,5 @@
> +  }
> +}
> +
> +NS_IMETHODIMP
> +ServiceWorkerMessageEvent::GetOrigin(nsAString& aOrigin)

void
ServiceWorkerMessageEvent::GetOrigin(nsAString& aOrigin) const

@@ +73,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +ServiceWorkerMessageEvent::GetLastEventId(nsAString& aLastEventId)

void
ServiceWorkerMessageEvent::GetLastEventId(nsAString& aLastEventId) const

@@ +90,5 @@
> +  }
> +}
> +
> +MessagePortList*
> +ServiceWorkerMessageEvent::GetPorts()

const

::: dom/workers/ServiceWorkerMessageEvent.h
@@ +23,5 @@
> +}
> +
> +class ServiceWorkerMessageEvent final : public Event
> +{
> +public:

NS_DECL_ISUPPORTS_INHERITED

@@ +38,5 @@
> +
> +  void GetData(JSContext* aCx, JS::MutableHandle<JS::Value> aData,
> +               ErrorResult& aRv);
> +
> +  NS_IMETHOD GetOrigin(nsAString & aOrigin);

nsAString& aOrigin

@@ +40,5 @@
> +               ErrorResult& aRv);
> +
> +  NS_IMETHOD GetOrigin(nsAString & aOrigin);
> +
> +  NS_IMETHOD GetLastEventId(nsAString & aLastEventId);

nsAString& aLastEventId

Comment 9

[Cătălin Badea (:catalinb)]

(In reply to Dimi Lee[:dimi][:dlee] from comment #6)
> Comment on attachment 8629317 [details] [diff] [review]
> (WIP)Patch v2
> 
> Hi Catalin,
> I wrote a WIP patch for serviceworkermessageevent, could you help take a
> look at it to make sure i am on the right track ? 
> 
> And I am not sure if there is any other place I need to create
> |serviceworkermessageevent|, now I only add it in ServiceWorkerClient.cpp
> Also not quite sure if I should set |source| and |ports|.
> 
> Thanks for help!

For now, just use it in ServiceWorkerClient::PostMessage and set the |source| and |ports| fields. Follow the specification at: http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#client-postmessage-method

I'm sorry for the delay!

Comment 10

[Dimi Lee [ooo 16.04 - 21.04］]

I am implementing ServiceWorkerMessageEvent interface according to :
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#serviceworkermessage-event-section

the |source| attribute defined in spec is
[SameObject] readonly attribute (ServiceWorker or MessagePort)? source;

But it will show this build error - "An attribute with [SameObject] must have an interface type as its type"
Do you have any suggestion for this ? thanks!

Comment 11

[Boris Zbarsky [:bzbarsky]]

Well, that IDL isn't actually valid IDL at the moment per spec.

So what needs to happen is to get the IDL spec changed and then get our implementation changed, or to get this IDL changed.  The relevant text at http://heycam.github.io/webidl/#SameObject is:

 The [SameObject] extended attribute MUST NOT be used on anything other than a read only
 attribute whose type is an interface type or object. 

I guess the right change would be to change this to include union types all of whose flat member types are interface types or object.

In the meantime, if you want to get this landed before the IDL spec is updated, you can either remove the annotation altogether or replace it with [Constant] and file a followup bug to do [SameObject] instead if/when the spec changes.  But please do file a spec issue on either webidl or serviceworkers, because one or the other needs to change here.

Comment 12

[Cătălin Badea (:catalinb)]

Dimi, do you have any updates on this bug? Thanks.

Comment 13

[Dimi Lee [ooo 16.04 - 21.04］]

ah, sorry i forget update the patch for review. I am PTO today and will update the patch tomorrow

Comment 14

[Dimi Lee [ooo 16.04 - 21.04］]

I am trying to remove the ServiceWorker occurrences from MessageEvent and found this should be done after replacing MessageEvent with ExtendableMessageEvent[1](Please correct me if I am wrong).

Do you think it is ok that I create another bug for ExtendableMessageEvent or it would be better to implement it in this bug?

[1]https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MessageEvent.webidl#37

Comment 15

[Cătălin Badea (:catalinb)]

This bug is fine, you can do it in a separate patch if it helps.

Comment 16

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part1. Add ServiceWorkerMessageEvent interface v1

Hi Andrea, could you help review this patch ? I will upload ExtendableMessageEvent change when i finish it.

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8649717 [details] [diff] [review]
Part1. Add ServiceWorkerMessageEvent interface v1

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

lgtm but I would like to have a feedback from smaug about this event an in particular about if this can be generated by the webIDL codegen.

::: dom/workers/ServiceWorkerClient.cpp
@@ +149,5 @@
> +    nsRefPtr<ServiceWorkerMessageEvent> event =
> +      ServiceWorkerMessageEvent::Constructor(aTargetContainer,
> +                                             NS_LITERAL_STRING("message"), init, rv);
> +
> +    if (rv.Failed()) {

NS_WARN_IF

::: dom/workers/ServiceWorkerMessageEvent.cpp
@@ +115,5 @@
> +  nsRefPtr<ServiceWorkerMessageEvent> event =
> +    new ServiceWorkerMessageEvent(aEventTarget, nullptr, nullptr);
> +
> +  aRv = event->InitEvent(aType, aParam.mBubbles, aParam.mCancelable);
> +  if (aRv.Failed()) {

NS_WARN_IF

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8649717 [details] [diff] [review]
Part1. Add ServiceWorkerMessageEvent interface v1

Given that other properties are simple and 
readonly attribute (ServiceWorker or MessagePort)? source; looks similar to
.track in TrackEvent, I think codegenerator should be able to deal with this just fine.

So, just add the .webidl file and then add the filename to the
GENERATED_EVENTS_WEBIDL_FILES in dom/webidl/moz.build and build.
You should get
<objdir>/dist/include/mozilla/dom/<YourEvent>.h and
<objdir>/dom/bindings/<YourEvent>.cpp in case you're curious to see what got generated.

Comment 19

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part2. WIP

Comment 20

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part2. Add ExtendableMessageEvent interface

Hi Andrea, could you help review this ? Thanks

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8652293 [details] [diff] [review]
Part2. Add ExtendableMessageEvent interface

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

::: dom/workers/ServiceWorkerEvents.cpp
@@ +505,5 @@
> +  } else if (mServiceWorker) {
> +    aValue.SetValue().SetAsServiceWorker() = mServiceWorker;
> +  } else if (mMessagePort) {
> +    aValue.SetValue().SetAsMessagePort() = mMessagePort;
> +  }

else {
  MOZ_CRASH("...");
}

@@ +536,5 @@
> +  event->SetTrusted(trusted);
> +
> +  event->mData = aOptions.mData;
> +
> +  mozilla::HoldJSObjects(event.get());

move this in the CTOR of ExtendableMessageEvent.

@@ +560,5 @@
> +  }
> +
> +  if (aOptions.mPorts.WasPassed() && !aOptions.mPorts.Value().IsNull()) {
> +    nsTArray<nsRefPtr<MessagePortBase>> ports;
> +    for (uint32_t i = 0, len = aOptions.mPorts.Value().Value().Length(); i < len; ++i) {

Use a variable instead Value().Value()...

::: dom/workers/WorkerPrivate.cpp
@@ +1160,5 @@
> +
> +      ErrorResult rv;
> +      nsRefPtr<ExtendableMessageEvent> event = ExtendableMessageEvent::Constructor(
> +        aTarget, NS_LITERAL_STRING("message"), init, rv);
> +      if (rv.Failed()) {

NS_WARN_IF

@@ +1176,5 @@
> +                                            messageData,
> +                                            EmptyString(),
> +                                            EmptyString(),
> +                                            nullptr);
> +      if (NS_FAILED(rv)) {

NS_WARN_IF

Comment 22

[Dimi Lee [ooo 16.04 - 21.04］]

update:
fixing an "postMessage" undefined build error on Windows platform

Comment 23

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part2. Add ExtendableMessageEvent interface v2

Hi Andrea,
Because in previous patch, there is build error if i include MessagePortList.h in ServiceWorkerEvent.cpp on windows platform(postmessage issue).

So I move extendableMessageEvent out of ServiceWorkerEvent.cpp
, could you help review this patch again ? thanks!

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8657770 [details] [diff] [review]
Part2. Add ExtendableMessageEvent interface v2

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

Looks good, but add a test, please.

::: dom/webidl/ExtendableMessageEvent.webidl
@@ +35,5 @@
> +};
> +
> +dictionary ExtendableMessageEventInit : ExtendableEventInit {
> +  any data;
> +  DOMString origin;

= "";

@@ +36,5 @@
> +
> +dictionary ExtendableMessageEventInit : ExtendableEventInit {
> +  any data;
> +  DOMString origin;
> +  DOMString lastEventId;

= "";

::: dom/workers/ExtendableMessageEvent.cpp
@@ +31,5 @@
> +  DropJSObjects(this);
> +}
> +
> +void
> +ExtendableMessageEvent::GetData(JSContext* aCx, JS::MutableHandle<JS::Value> aData,

JS::MutableHandle in a new line.

@@ +104,5 @@
> +  }
> +
> +  if (aOptions.mPorts.WasPassed() && !aOptions.mPorts.Value().IsNull()) {
> +    nsTArray<nsRefPtr<MessagePortBase>> ports;
> +    Sequence<OwningNonNull<MessagePortBase>> portsParam =

Sequence<OwningNonNull<MessagePortBase>>& portsParam =

::: dom/workers/ExtendableMessageEvent.h
@@ +10,5 @@
> +#include "mozilla/dom/Event.h"
> +#include "mozilla/dom/ExtendableMessageEventBinding.h"
> +
> +//#include "ServiceWorker.h"
> +//#include "ServiceWorkerClient.h"

why these 2 commented out? Remove them if you don't need them.

@@ +46,5 @@
> +                                                         ExtendableEvent)
> +
> +  NS_FORWARD_TO_EVENT
> +
> +  virtual JSObject* WrapObjectInternal(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override

80chars.

@@ +68,5 @@
> +               ErrorResult& aRv);
> +
> +  void GetSource(Nullable<OwningClientOrServiceWorkerOrMessagePort>& aValue) const;
> +
> +  NS_IMETHODIMP GetOrigin(nsAString& aOrigin)

NS_IMETHOD

@@ +74,5 @@
> +    aOrigin = mOrigin;
> +    return NS_OK;
> +  }
> +
> +  NS_IMETHODIMP GetLastEventId(nsAString& aLastEventId)

NS_IMETHOD

@@ +95,5 @@
> +    mServiceWorker = aServiceWorker;
> +  }
> +};
> +
> +END_WORKERS_NAMESPACE

empty line after END_WORKER_NAMESPACE.

::: dom/workers/WorkerPrivate.cpp
@@ +651,5 @@
> +      init.mBubbles = false;
> +      init.mCancelable = true;
> +
> +      init.mData = messageData;
> +      init.mOrigin.Construct(EmptyString());

if you set the default value of origin and lastEventId to "" in the dictionary, you don't need to do this.

Comment 25

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part1. Add ServiceWorkerMessageEvent interface v2

- rebase
- address comment 17

Comment 26

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: ServiceWorkerMessageEvent interface v3

I encounter postmessage build error on windows platform after rebasing ExtendableMessageEvent patch to latest code. It will take some times to figure that out and I would like to land ServiceWorkerMessageEvent interface first so remove part2 and create bug 1207068

Upload rebase patch and following is try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7239fafe39fd

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54e813c7674c

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/54e813c7674c

Comment 29

[Chris Mills [:cmills]]

I'm going to document this interface soon, but I just wanted to clarify something, to make sure I'm getting this right.

This is to do with a message channel involving a service worker, right? when onmessage is fired over in the service worker, its event object will be an ExtendableMessageEvent instance, right? And when onmessage is fired over in the main context, its event object will be a ServiceWorkerMessageEvent instance, correct?

I'm just checking, because logging the event objects to the console seem to give slightly odd results: ExtendableMessageEvent over in the SW, but a message {} object with access to all its members in the main thread.

Comment 30

[Chris Mills [:cmills]]

I'm going to document this interface soon, but I just wanted to clarify something, to make sure I'm getting this right.

This is to do with a message channel involving a service worker, right? when onmessage is fired over in the service worker, its event object will be an ExtendableMessageEvent instance, right? And when onmessage is fired over in the main context, its event object will be a ServiceWorkerMessageEvent instance, correct?

I'm just checking, because logging the event objects to the console seem to give slightly odd results: ExtendableMessageEvent over in the SW, but a message {} object with access to all its members in the main thread.

Comment 31

[Chris Mills [:cmills]]

Ok, we've got this documented, see here (and subpages):

https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerMessageEvent

And I've created a page for the relevant event:

https://developer.mozilla.org/en-US/docs/Web/Events/message_(ServiceWorker)

An appropriate entry has been added to the 44 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/44$edit

Let me know if this is ok.