Open Bug 1251707 Opened 9 years ago Updated 2 years ago

compressall causes unrelated IPC messages to be executed earlier

Categories

(Core :: IPC, defect, P3)

defect

Tracking

()

People

(Reporter: BenWa, Unassigned)

References

Details

(Whiteboard: btpp-backlog)

In this code: http://mxr.mozilla.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#713 We find the single message of the right type. Then we delete that message and insert the resulting compressed message at the back of the queue. However the message we deleted had called |new DequeueTask(mDequeueOneTask)|. This mDequeueOneTask is still in the queue however the message it correspond to is now deleted. That task now corresponds to the next message in the queue, which can be anything. This means that this message will now be executed earlier than it was queued. Essentially all the messages between the first and the current message will be executed by the earlier mDequeueOneTask. This will correct itself once we get to the end since we don't insert a new task. This is bad because when we post a message we assume that any pending queued events are processed. However this bug is causing us to run IPC messages before all queued events have been processed breaking this guarantee. NOTE: This isn't a bug for 'compress' (not all) since there not other messages in between that would get run earlier. This is a bit hard to explain so let me know if you need clarification.
Depends on: 1155494
Flags: needinfo?(mrbkap)
Whiteboard: btpp-followup-2016-03-04
OK, I think I understand the problem. Am I right that this only affects protocols that connect two threads in the same process? The only usage of compressall right now is in PBrowser, which is cross-process, so I don't think this is a problem yet. Benoit, are you still considering changing how compressall works? That would also fix this I think.
Flags: needinfo?(bgirard)
(In reply to Bill McCloskey (:billm) from comment #1) > Am I right that this only affects > protocols that connect two threads in the same process? That's not obvious to me. Can you elaborate? > > Benoit, are you still considering changing how compressall works? That would > also fix this I think. Even with the stronger 'compressfirst' semantic I implemented I'm not able to get my messages on time. They're sitting in the queue with no message to compress against and we end up painting with a very stale scrolling position (off by 3,000 pixels which is huge). So ATM I'm not considering changing compression.
Flags: needinfo?(bgirard)
(In reply to Benoit Girard (:BenWa) from comment #2) > (In reply to Bill McCloskey (:billm) from comment #1) > > Am I right that this only affects > > protocols that connect two threads in the same process? > > That's not obvious to me. Can you elaborate? > We discussed this on IRC. We agreed that it can happen with e10s with a single channel. The best example I saw was this code: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorChild.cpp#110 It make assumptions that the stop response messages are being executed before executing PostTask 'DeferredDestroyCompositor'. This bug might cause some messages to run that would of otherwise run after DeferredDestroyCompositor. However I can't pin-point anything that would definitively break and it's not trivial to write that code by mistake. We agreed that this bug is worth fixing but it's might not be causing problems for us ATM.
It looks like billm already answered the needinfo here.
Flags: needinfo?(mrbkap)
Whiteboard: btpp-followup-2016-03-04 → btpp-backlog
Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.