Bug 254777 - REGRESSION (Safari 16.4): PostMessage with transfer object is broken between contexts
Summary: REGRESSION (Safari 16.4): PostMessage with transfer object is broken between ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 16
Hardware: All All
: P2 Major
Assignee: Chris Dumez
URL:
Keywords: BrowserCompat, InRadar
Depends on:
Blocks:
 
Reported: 2023-03-30 15:09 PDT by Justin Mayfield
Modified: 2023-07-20 10:42 PDT (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Mayfield 2023-03-30 15:09:03 PDT
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.
Comment 1 Justin Mayfield 2023-03-30 15:53:49 PDT
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.
Comment 2 Karl Dubost 2023-04-02 17:49:53 PDT
It would be nice to have a simplified testcase
Comment 3 Justin Mayfield 2023-04-02 18:18:47 PDT
(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.
Comment 4 Alexey Proskuryakov 2023-04-02 18:34:14 PDT
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.
Comment 5 Radar WebKit Bug Importer 2023-04-02 18:34:24 PDT
<rdar://problem/107538083>
Comment 6 Justin Mayfield 2023-04-02 18:55:20 PDT
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.
Comment 7 Karl Dubost 2023-04-09 19:12:52 PDT
Justin Mayfield,

does it work if you exclude the port in the data argument of postMessage?
Comment 8 Justin Mayfield 2023-04-09 19:29:35 PDT
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];
Comment 9 John Wilander 2023-04-11 16:34:58 PDT
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!
Comment 10 Justin Mayfield 2023-04-11 17:32:36 PDT
(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
Comment 11 Hiroki Kato 2023-04-18 17:58:35 PDT
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
Comment 12 Justin Mayfield 2023-04-18 18:16:42 PDT
(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!
Comment 13 Ali Juma 2023-04-19 05:57:56 PDT
(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.
Comment 14 Ali Juma 2023-04-19 05:58:32 PDT
(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
Comment 15 Chris Dumez 2023-04-19 13:17:11 PDT
Pull request: https://github.com/WebKit/WebKit/pull/12929
Comment 16 EWS 2023-04-19 16:39:58 PDT
Committed 263155@main (ca6ca7d1895d): <https://commits.webkit.org/263155@main>

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