1204775 - SharedWorker.port should be a 'real' MessagePort.

Status: RESOLVED FIXED

Description

[Andrea Marchesini [:baku]]

Attachment: mp.patch

Comment 1

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Comment on attachment 8661056 [details] [diff] [review]
mp.patch

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

::: dom/base/StructuredCloneHelper.h
@@ +190,5 @@
>  
>    // This must be called if the transferring has ports generated by Read().
>    // MessagePorts are not thread-safe and they must be retrieved in the thread
>    // where they are created.
> +  void TakeTransferredPorts(nsTArray<nsRefPtr<MessagePort>>& aPorts)

While we're here, make this take a move reference.

@@ +195,4 @@
>    {
>      MOZ_ASSERT(mSupportsTransferring);
>      MOZ_ASSERT(aPorts.IsEmpty());
>      aPorts.SwapElements(mTransferredPorts);

And then just assign here.

::: dom/messagechannel/MessagePort.h
@@ +60,5 @@
>  
>    virtual void
>    PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
>                const Optional<Sequence<JS::Value>>& aTransferable,
> +              ErrorResult& aRv);

Why are any of these functions still virtual?

::: dom/workers/ServiceWorkerRegistrar.cpp
@@ +10,5 @@
>  #include "nsIEventTarget.h"
>  #include "nsIInputStream.h"
>  #include "nsILineInputStream.h"
>  #include "nsIObserverService.h"
> +#include "nsIOutputStream.h"

...?

::: dom/workers/WorkerPrivate.cpp
@@ +1503,5 @@
> +                      MessagePort* aPort)
> +  : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount)
> +  {
> +    MOZ_ASSERT(aPort);
> +    // In order to move the port from 1 thread to the another one, we have to

s/1/one/ and then "the another" should become "the other" or "another"

Comment 2

[Andrea Marchesini [:baku]]

Attachment: mp.patch

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

Comment 3

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccfddcbccdac

Comment 4

[Wes Kocher (:KWierso)]

I had to back this out for various bustages: https://hg.mozilla.org/integration/mozilla-inbound/rev/27cdeffa4ae1

https://treeherder.mozilla.org/logviewer.html#?job_id=14146579&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=14146100&repo=mozilla-inbound

Comment 5

[Wes Kocher (:KWierso)]

(In reply to Wes Kocher (:KWierso) from comment #4)
> I had to back this out for various bustages:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/27cdeffa4ae1
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=14146579&repo=mozilla-
> inbound
> https://treeherder.mozilla.org/logviewer.html#?job_id=14146100&repo=mozilla-
> inbound

Oops, this was supposed to go to bug 1203463.

Comment 6

[Wes Kocher (:KWierso)]

Except that mulet m(4) IS from this patch, so it's backed out too:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad7f8d8c042

Comment 7

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/faf94ffc0c5a

Comment 8

[Wes Kocher (:KWierso)]

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

Comment 9

[Wes Kocher (:KWierso)]

I think this is responsible for the spike in both bug 1189886 and bug 1168411.

Comment 10

[Andrea Marchesini [:baku]]

Yes, it is and I'm actively working on it.