Bug 254777 – REGRESSION (Safari 16.4): PostMessage with transfer object is broken between contexts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Justin Mayfield]

Sending a transferable object like a MessagePort to communicate between different contexts no longer works properly.  When I use the transfer argument to postMessage the event handler gets an event with a null data property.

Steps to reproduce:

1. Call postMessage({foo: 'bar', port}, self.origin, [port]) from page script.
2. Add window.message event handler to a different context such as an
   extension's content script context and evaluate the event callback arg.
3. Observe the ev.data is null instead of {foo: 'bar', port: port}

Expected behavior:
For the message handler to get an event object with a non-null data property filed with the contents sent from the postMessage caller.

I first noticed this on the Orion Safari fork and filed a bug with them, https://orionfeedback.org/d/4730-postmessage-with-transfer-object-is-broken, but after updating to Safari 16.4 on the macOS 13.3 I see the same bug there, which leads me to believe this is a core WebKit regression.  This is directly affecting my Safari extension which no longer works on anyone's 16.4 based Safari browsers.  The same code works fine on all Chromium and Firefox.

[Justin Mayfield]

More observations.

1. I can only reproduce with MessagePort transferables.  Using ArrayBuffer for example works as expected.
2. The data property of the MessageEvent is null but the ports[] array is populated with the MessagePort object.
3. Excluding the MessagePort from the sending side's data object works as expected.  The data property of the MessageEvent is correctly set to the cloned object and the ports[] property contains the transferred MessagePort.


I'll be refactoring my use case to use the technique from #3;  It's slightly less safe because I need to make assumptions about the ordering of the ports[] property on the receiver side instead of being able to directly access the transferables from the data object property but at least I'll be able to get my Safari 16.4 users working again and support Orion.

[Karl Dubost]

It would be nice to have a simplified testcase

[Justin Mayfield]

(In reply to Karl Dubost from comment #2)
> It would be nice to have a simplified testcase

I'm not familiar enough with WebKit to simulate an extension's content-script context.  I tried a simple test with a Worker and an iframe but neither are affected.

[Alexey Proskuryakov]

Would it be possible to provide easy to reproduce test steps, even if not minimized? This way, someone who isn't deeply familiar with the code could e.g. bisect to find the culprit change.

[Radar WebKit Bug Importer]

<rdar://problem/107538083>

[Justin Mayfield]

Safari requires a significant amount of tooling and xcode interaction so I'll just skip that, but with Orion you can create a local extension folder and populate it with a manifest.json such as this...

  {
    "manifest_version": 2,
    "content_scripts": [{
        "run_at": "document_start",
        "matches": ["*://*/*"],
        "js": ["ext-context.js"]
    }]
    ...
  }


ext-context.js should do this...

  addEventListener('message', ev => {
    if (ev.data && ev.data.port instanceof MessagePort) {
        console.info("pass");
    } else {
        console.error("fail");
    }
  });



And then visit a mock page that runs this script...

  const ch = new MessageChannel();
  const port = ch.port2;
  postMessage({port}, self.origin, [port]);


I'll see if I can find where the regression came in too;  I realize this is probably not helpful.

[Karl Dubost]

Justin Mayfield,

does it work if you exclude the port in the data argument of postMessage?

[Justin Mayfield]

Yes, and that is the exact workaround I'm using in my extension.  It only works because there is the .ports[] array to save the day when you transfer a MessagePort specifically.  But then again, this only seems to affect MessagePort transfer objects.

This is pattern I'm using now that works on all browsers..
    const transfer = [port];
    postMessage({portIndex: transfer.indexOf(port)}, transfer);

And from my handler I do..
    const port = event.ports[event.data.portIndex];

[John Wilander]

Hi, Justin! Thanks for filing. Could you share which extension this is for? And am I right in that you have already deployed a workaround so the bug won't reproduce with your extension? Thanks!

[Justin Mayfield]

(In reply to John Wilander from comment #9)
> Hi, Justin! Thanks for filing. Could you share which extension this is for?
> And am I right in that you have already deployed a workaround so the bug
> won't reproduce with your extension? Thanks!

Hello John,

This is my extension:
   https://github.com/SauceLLC/sauce4strava
   https://apps.apple.com/us/app/sauce-for-strava/id1570922521
   https://addons.mozilla.org/en-US/firefox/addon/sauce4strava/
   https://chrome.google.com/webstore/detail/eigiefcapdcdmncdghkeahgfmnobigha

And you are correct, I already released patched builds (v8.2.2) on the Apple and Firefox stores.

This is the commit with my workaround: https://github.com/SauceLLC/sauce4strava/commit/9100e2db6245b5db7bf104b3044029d0b936ec7a

I don't know how to get you a Safari based build with the old behavior from the app-store, but if you use Onion you can install this unpatched firefox build: https://addons.mozilla.org/firefox/downloads/file/4078653/sauce4strava-8.2.1.xpi

I should warn you that you'll probably need to create an account with strava.com first.  It's free, but there would be some tedium in trying to repro from my particular extension.  You'll also need to be on a page that invokes these postMessage calls, such as this example: https://www.strava.com/activities/8355858982

[Hiroki Kato]

Hello.

This issue probably affects Chrome iOS as well. I encountered a similar situation, and at first I thought it was a problem on Chrome's side, but it seems that a change on the WebKit side was having an effect instead.

https://bugs.chromium.org/p/chromium/issues/detail?id=1431021

[Justin Mayfield]

(In reply to Hiroki Kato from comment #11)
> Hello.
> 
> This issue probably affects Chrome iOS as well. I encountered a similar
> situation, and at first I thought it was a problem on Chrome's side, but it
> seems that a change on the WebKit side was having an effect instead.
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=1431021

Very interesting.  This comment in particular is very interesting: https://bugs.chromium.org/p/chromium/issues/detail?id=1431021#c6

Thanks for cross referencing here Hiroki!

[Ali Juma]

(In reply to Justin Mayfield from comment #12)
> (In reply to Hiroki Kato from comment #11)
> > Hello.
> > 
> > This issue probably affects Chrome iOS as well. I encountered a similar
> > situation, and at first I thought it was a problem on Chrome's side, but it
> > seems that a change on the WebKit side was having an effect instead.
> > 
> > https://bugs.chromium.org/p/chromium/issues/detail?id=1431021
> 
> Very interesting.  This comment in particular is very interesting:
> https://bugs.chromium.org/p/chromium/issues/detail?id=1431021#c6

https://bugs.chromium.org/p/chromium/issues/detail?id=1431021#c4b also has a small test app and test page.

[Ali Juma]

(In reply to Ali Juma from comment #13)
> (In reply to Justin Mayfield from comment #12)
> > (In reply to Hiroki Kato from comment #11)
> > > Hello.
> > > 
> > > This issue probably affects Chrome iOS as well. I encountered a similar
> > > situation, and at first I thought it was a problem on Chrome's side, but it
> > > seems that a change on the WebKit side was having an effect instead.
> > > 
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=1431021
> > 
> > Very interesting.  This comment in particular is very interesting:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=1431021#c6
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=1431021#c4b also has a
> small test app and test page.

Sorry, typo, I meant https://bugs.chromium.org/p/chromium/issues/detail?id=1431021#c4

[Chris Dumez]

Pull request: https://github.com/WebKit/WebKit/pull/12929

[EWS]

Committed 263155@main (ca6ca7d1895d): <https://commits.webkit.org/263155@main>

Reviewed commits have been landed. Closing PR #12929 and removing active labels.