Closed Bug 1315913 Opened 3 years ago Closed 3 years ago

Refactor nsFrameMessageManager's BuildClonedMessageDataFor*/UnpackClonedMessageDataFor* to live on StructuredCloneData and add support for PBackground variants and convert BroadcastChannel and MessageChannel to use it

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: asuth, Assigned: asuth)

References

Details

Attachments

(3 files)

While trying to figure out how to remote the ServiceWorker postMessage() variant across processes for bug 1231216 I observed that:
- nsFrameMessageManager already had a reusable traits-based mechanism for the PContent case for converting to/from ClonedMessageData in the form of its BuildClonedMessageDataFor*/UnpackClonedMessageDataFor* helpers.  But it required some hacking for external code to use it, and it does not support PBackground.
- BroadcastChannel uses (non-templated, manually-type-specific) effectively identical logic for converting to/from ClonedMessageData for PBackground except for skipping the port identifiers.
- MessageChannel/MessagePort's MessagePortMessage IPDL type is effectively the same type as ClonedMessageData and also uses (non-templated, manually-type-specific) effectively identical logic for its PBackground case.
- The ServiceWorkerPrivate remoting case currently needs to be PContent-based, but will ideally shift to be PBackground-based, so although the existing nsFrameMessageManager could be used with a hack, that would be a stumbling block in a migration to PBackground.

So I've speculatively made a patch series that:
1. (bullet points!)
  - Moves the trait-based build/unpack mechanism from nsFrameMessageManager.cpp to live on StructuredCloneData.
  - Adds support for PBackground to the template magic.
  - In the unpack case adds support for 3 memory strategies: borrow (aka UseExternalData), copy (aka CopyExternalData), steal (new!).  Steal is necessary whenever the Read() call may occur after the Recv* method returns.  For the build-case we are able to hand-wave that borrowing is always okay because the life-cycles can be controlled in this case since you're not at the mercy of the Recv*'s (IPC) caller.
  - Introduces a StructuredCloneDataNoTransfers subclass for the BroadcastChannel use-case.
  - Leaves the nsFrameMessageManager.cpp Build/Unpack methods in-place but hollows them out to use the methods on StructuredCloneData
  - Adds hopefully useful comments.
2. Converts BroadcastChannel
3. Converts MessageChannel.  This patch is a little more chatty because of the need to s/MessagePortMessage/ClonedMessageData/ and because the previous idiom was stealing the buffers when converting to MessagePortMessage and, as noted above, we only borrow right now, so some reordering and comments were required.  The most elegant solution to all of this might just be to have SerializedStructuredCloneBuffer hold SharedJSAllocatedData instances instead of directly holding JSStructuredCloneData instances.

Patches coming up in a sec.  "mach test dom/broadcastchannel" and "mach test dom/messagechannel" pass locally on a debug build, but there could be issues.  Going to also do a try push.
Comment on attachment 8808545 [details] [diff] [review]
Part 1: Move nsFrameMessenger structured clone Build/Unpack into StructuredCloneData and expand to support PBackground.

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

::: dom/ipc/StructuredCloneData.h
@@ +20,5 @@
>  namespace mozilla {
> +namespace ipc {
> +
> +class PBackgroundParent;
> +class PBackgroundChild;

alphabetic order.

@@ +28,4 @@
>  namespace dom {
> +
> +class nsIContentParent;
> +class nsIContentChild;

same here.
Attachment #8808545 - Flags: review?(amarchesini) → review+
Attachment #8808547 - Flags: review?(amarchesini) → review+
Attachment #8808548 - Flags: review?(amarchesini) → review+
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06e8705b9542
Part 1: Move nsFrameMessenger structured clone Build/Unpack into StructuredCloneData and expand to support PBackground. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4bdab06769
Part 2: Convert BroadcastChannel to use StructuredCloneDataNoTransfers instead of copy-and-paste. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb14c93e197
Part 3: Convert MessageChannel to use StructuredCloneData and ClonedMessageData. r=baku
https://hg.mozilla.org/mozilla-central/rev/06e8705b9542
https://hg.mozilla.org/mozilla-central/rev/ae4bdab06769
https://hg.mozilla.org/mozilla-central/rev/1eb14c93e197
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.