Implement the ServiceWorkerMessageEvent interface

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: catalinb, Assigned: dimi)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla44
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

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

Comment 2

4 years ago
I would like to work on this bug, so the purpose of this bug is to implement serviceworkermessageevent & extendablemessageevent ?
(Assignee)

Comment 3

4 years ago
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 ?
Flags: needinfo?(catalin.badea392)
(Assignee)

Comment 4

4 years ago
Posted patch (WIP)Patch v1 (obsolete) — Splinter Review
(Assignee)

Comment 5

4 years ago
Posted patch (WIP)Patch v2 (obsolete) — Splinter Review
Attachment #8629316 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
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!
Attachment #8629317 - Flags: feedback?(catalin.badea392)
(Reporter)

Comment 7

4 years ago
(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.
Flags: needinfo?(catalin.badea392)
Summary: Update MessageEvent.source to be compliant with the spec → Implement the ServiceWorkerMessageEvent interface
(Reporter)

Comment 8

4 years ago
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
Attachment #8629317 - Flags: feedback?(catalin.badea392) → feedback+
(Reporter)

Comment 9

4 years ago
(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!
(Assignee)

Updated

4 years ago
Assignee: nobody → dlee
(Assignee)

Comment 10

4 years ago
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!
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 12

4 years ago
Dimi, do you have any updates on this bug? Thanks.
Flags: needinfo?(dlee)
(Assignee)

Comment 13

4 years ago
ah, sorry i forget update the patch for review. I am PTO today and will update the patch tomorrow
Flags: needinfo?(dlee)
(Assignee)

Comment 14

4 years ago
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
Flags: needinfo?(catalin.badea392)
(Reporter)

Comment 15

4 years ago
This bug is fine, you can do it in a separate patch if it helps.
Flags: needinfo?(catalin.badea392)
(Assignee)

Updated

4 years ago
Blocks: 1196097
(Assignee)

Comment 16

4 years ago
Hi Andrea, could you help review this patch ? I will upload ExtendableMessageEvent change when i finish it.
Attachment #8629317 - Attachment is obsolete: true
Attachment #8649717 - Flags: review?(amarchesini)
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
Attachment #8649717 - Flags: review?(amarchesini) → feedback?(bugs)
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.
Attachment #8649717 - Flags: feedback?(bugs) → feedback+
(Assignee)

Comment 19

4 years ago
Posted patch Part2. WIP (obsolete) — Splinter Review
(Assignee)

Comment 20

4 years ago
Hi Andrea, could you help review this ? Thanks
Attachment #8650947 - Attachment is obsolete: true
Attachment #8652293 - Flags: review?(amarchesini)
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
Attachment #8652293 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 22

4 years ago
update:
fixing an "postMessage" undefined build error on Windows platform
(Assignee)

Comment 23

4 years ago
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!
Attachment #8652293 - Attachment is obsolete: true
Attachment #8657770 - Flags: review?(amarchesini)
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.
Attachment #8657770 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 25

4 years ago
- rebase
- address comment 17
Attachment #8649717 - Attachment is obsolete: true
Attachment #8658600 - Flags: review+
Status: NEW → ASSIGNED
(Assignee)

Comment 26

4 years ago
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
Attachment #8657770 - Attachment is obsolete: true
Attachment #8658600 - Attachment is obsolete: true
Attachment #8664122 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/54e813c7674c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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.
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.
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.
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1154754

Updated

3 years ago
Depends on: 1259164
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.