526361 - Defer "nonqueued" messages received during synchronous IPC calls

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Sending all messages to DefWndProc is not great long-term. We should instead queue the messages we receive and then fake them once we get back to the event loop. Patch attached does exactly that.

The windows-only code here has grown large enough that I think it should be in its own file. I'll move some stuff over to a new file in a new patch in a sec, but that introduces lots of noise. This current patch is the meat of the changes.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

Ok, here's the first patch (plus a few little tweaks) moved into its own file. This cleans up SyncChannel quite a bit.

The basic strategy is to grab all the messages that we process during the neutered window procedure and stuff fake runnables into an array. Then we set a hook once we leave the WaitForNotify function to run before any other messages can. That hook runs all the deferred messages and cleans up after itself. Simple! ;)

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Debug patch for auditing "nonqueued" messages

Here's the patch I used to figure out which messages we might need to handle.

Comment 3

[Jim Mathies [:jimm]]

ben, can you do a quick merge and repost? I can look at this today.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2, refreshed

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 412006 [details] [diff] [review]
Patch, v2, refreshed

Generally the code looks good. Does this put parent stability at risk if the child misbehaves? I wonder if we should have a max queue depth in place. Also, in cases where you are re-firing deferred messages, I would not assume the window / window procedure is valid. So for example, 

void
DeferredSendMessage::Run()
{
  AssertWindowIsNotNeutered(hWnd);
  WNDPROC wndproc =
    reinterpret_cast<WNDPROC>(GetWindowLongPtr(hWnd, GWLP_WNDPROC));
  CallWindowProc(wndproc, hWnd, message, wParam, lParam);
}

should check the result of GetWindowLongPtr.

Other than that, looks ok to me.

Comment 6

[Jim Mathies [:jimm]]

IsWindow(hwnd) might be a good call to add for those hwnd operations as well.

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 412006 [details] [diff] [review]
Patch, v2, refreshed

Looks fine with Jim's comments taken into account.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ok, I bullet-proofed everything. Thanks guys!

Pushed changeset 6b3ac4cfff1c to electrolysis.

robarnold, feel free to file a followup if you have any concerns.