SharedWorker.port should be a 'real' MessagePort.

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch mp.patch (obsolete) — Splinter Review
No description provided.
Attachment #8661056 - Flags: review?(khuey)
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"
Attachment #8661056 - Flags: review?(khuey) → review+
Keywords: checkin-needed
(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.
Flags: needinfo?(amarchesini)
Except that mulet m(4) IS from this patch, so it's backed out too:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad7f8d8c042
Flags: needinfo?(amarchesini)
Blocks: 1193414
https://hg.mozilla.org/mozilla-central/rev/faf94ffc0c5a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
I think this is responsible for the spike in both bug 1189886 and bug 1168411.
Blocks: 1189886, 1168411
Flags: needinfo?(amarchesini)
Yes, it is and I'm actively working on it.
Flags: needinfo?(amarchesini)
Duplicate of this bug: 667501
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.