Safari bug: Hover tooltips not working

[airamare01]

Reported by https://github.com/sourcegraph/accounts/issues/545

• Sourcegraph version: 5.x
• Platform information: Safari browser.

Steps to reproduce:

1. Open up GitHub in Safari.
2. Navigate to a file of choice on any repo.
3. Hover cursor on code.
4. Repeat the same steps in Chrome or Firefox.

Loom video to showcase behavior here

Expected behavior:

Hover tooltips pop up for code intelligence.

Actual behavior:

No hover tooltips.

If you would like immediate help on this, please email support@sourcegraph.com (you can still create the issue, but there are no SLAs on issues like there are for support requests).

[taras-yemets]

I did some debugging, and it seems that the initialize extension host promise is never resolved in Safari. Thus extensionsController.extHostAPI promise is not resolved either.

[taras-yemets]

@valerybugakov and I looked closely into this issue, and here are our findings.

We use comlink to handle communication between web workers.
The extension host API factory is exposed from the background thread and then the exposed value is accessed in the main thread.
We added a few breakpoints to the comlink code (postMessage call in the background thread and its handler in the main thread), ran the code in Chrome and Safari and discovered the following.

1. When posting a message from the background thread to the main in Chrome, the main thread receives the expected ev.data value (of type handler in this case).

Screenshots

2. In Safari ev.data for the event with type handler is always null, and the message event listener silently exits.

Screenshots

3. Events with both handler and raw types have the expected data value in Chrome, but only events with type raw have the proper value in Safari.

4. MessagePort we send in the background thread is not closed (closeEnpdoint is not called) before we receive the message in the main thread.

It seems that Safari incorrectly handles MessageEvents with the handler type coming from the comlink expose method.
A potential further debugging/fixing step may be omitting using comlink and creating a MessagePort in the background thread (instead of using comlink's expose method) and using it to communicate with the main thread. Though this step may be quite time-consuming as we'll need to reimplement most parts of the comlink's expose and wrap methods.

[taras-yemets]

I've recorded a few more debug steps in Chrome and Safari.

https://www.loom.com/share/eb6f96aaf1e045a198f5178937a5ecd9

cc: @valerybugakov

[valerybugakov]

@taras-yemets, I've viewed the video you shared, and it's great to see that we're receiving the data in the main thread! However, it appears we're not successfully using MessagePorts in the same thread to transmit the data from the adapter to the Comlink message handler.

Despite revisiting the WebKit bug tracker, I wasn't able to find any relevant bugs, which left me scratching my head.

One question that arose was whether the type of data we're trying to send could influence this behavior. For instance, would we be able to hardcode a simple string or an object with a single field as a message and then receive it in the Comlink handler? Also, would there be a noticeable difference if we were to eliminate the transfer argument from adapterPort.postMessage()? Though I've gone through the MDN docs on the postMessage method's second argument, I'm not sure why we need it.

If these inquiries don't lead us to any solid conclusions, it might be beneficial to streamline the link function. We could achieve this by commenting out as much of the logic as we can, effectively bringing it closer to the minimal working example you created from the Comlink docs, which we know works in Safari. My thinking here is that by stripping down the message port manipulation logic in the main thread to its absolute essentials, we can avoid overlooking any crucial details.

Given that we've managed to narrow down the logic to two browser-API calls, our next step is to ensure that everything surrounding them, including arguments and their generation, is correct. If this approach doesn't provide us with the answers we're seeking, we should be left with a reproducible bug that we can bring to Apple's attention.

[taras-yemets]

Comlink in Safari fails to handle a message (exits if message event data is null) if the event.data.value is MessagePort (we explicitly set MessagePort as event data value here).

If we replace the MessagePort value with a string, for example (add the following before sending a message)

if (message.value instanceof MessagePort) {
    message.value = 'Hello world!'
}

Comlink's message handler receives the expected event.data value.

---

I've created a minimal working example and discovered the following:

1. Hooking into postMessage (commit) and adding a MessagePort to event data errors out in both browsers. It’s basically what we do here: replace the value at the path with a MessagePort and then send this message.
   

2. Logging the event data (commit) confirms that comlink in Safari handles events with type HANDLER and MessagePort as value as expected.

Main thread receives (commit) the same message as the background script sends.

It probably means that when comlink posts a message with a MessagePort inside (wireValue is a MessagePort - proxyTransferHandler) it’s handled correctly in the receiving thread. When we manually hook into the post message process - it fails.

[taras-yemets]

In the Sourcegrph browser extension we use browserPortToMessagePort function to establish the connection between browser.runtime.Port and MessagePort so that the latter can be used as an endpoint with comlink over browser extension script boundaries. After the connection is established and ports are linked, when receiving a message from the browser port we:

• create a new MessageChannel
• add port1 of this MessageChannel under the value key to the received browser port message
• call postMessage on the MessagePort used to communicate with comlink with the following arguments:
  • message: the modified message with port1 (MessagePort) under the value key
  • transferList: array with a single item port1 (MessagePort).
    As we identified earlier (see comments above), this is the place where we fail to handle messages from the background script.

We had a productive debugging session with @valerybugakov, and we narrowed the issue even more.

1. We've set up a minimal example of one MessagePort posting a message with another MessagePort and transferring another MessagePort in the browser main thread.
   When running this example in Chrome and Safari, we got the Incoming message on port1: {value: MessagePort} in the browser console.

2. We've set up a similar example, but in the browser extension content script, and got the following messages logged to the console:

• Chrome: Incoming message on port1: {value: MessagePort}
• Safari: Incoming message on port1: null

These minimal examples demonstrate that Safari handles sending MessagePort using postMessage differently depending on where it's executed: in the browser main thread or in the browser extension content script.

cc: @airamare01

[airamare01]

Thanks for the detailed explanation here @taras-yemets.
A quick question, since the two browsers have different paths of logic that handle MessagePort, is there a unifying way to address both in the logic flow?

[valerybugakov]

@airamare01, could you elaborate, please? I'm confused about the part where you mention "different paths". The logic is the same for both browsers. The problem is that it works in Chrome but fails in Safari unexpectedly. We can also discuss the details in Slack if it helps.