fix: ensure correct ordering of sendSync w.r.t. send #18630
Conversation
nornagon
force-pushed
the
ipc-ordering-fix
branch
from
7516dfb to
3b4e122
Compare
| const std::string& channel, | ||
| const base::ListValue& arguments) { | ||
| electron_browser_ptr_->Message(internal, channel, arguments.Clone()); | ||
| electron_browser_ptr_->get()->Message(internal, channel, arguments.Clone()); |
There was a problem hiding this comment.
Now that the interface ptr is created on the background runner, which is a different sequence to that of the main runner. Is it okay to call methods on this ptr in the main sequence without calling PassInterface ? well that would technically create a new pipe which would defeat the purpose. Just to make sure is this a safe operation ?
There was a problem hiding this comment.
The type of this pointer changed to ThreadSafeElectronBrowserPtr, whose raison d'etre is to allow calling methods from multiple threads. This doesn't create any new pipes.
There was a problem hiding this comment.
Ah okay it uses mojo/public/cpp/bindings/thread_safe_interface_ptr.h , wasn't aware of this helper.
There was a problem hiding this comment.
I wasn't either until I asked in #mojo on the Chromium Slack :)
|
I'm merging this ahead of review because it fixes a very annoying flake on master, but I'm happy to incorporate feedback from @zcbenz or anything further from @deepak1556 :) |
|
No Release Notes |
|
Edit: this seems to be related to #18555.
|
Description of Change
#18449 introduced a potential race between
sendandsendSyncby pushingsendSynccalls to a separate mojo pipe, which is the cause of the recent test flake inremote moduletests. This change rearranges the calls sosendand family all happen on the same pipe, and onlyinvokehappens on a different pipe, which it must do because it has a callback with a result (which can't be pending whensendSyncis called, or else DCHECK). There are no ordering concerns currently w/invokebecause we don't really use it, but in future we probably want to guarantee thatsendandinvokeshare an ordering, which will require some further trickery.Checklist
npm testpassesRelease Notes
Notes: no-notes