1331674 - (SyncIPC) [meta] Stop doing sync IPC from the content process or the UI thread

Status: NEW

(Core :: DOM: Core & HTML, defect, P3)

Description

[(no longer active)]

This bug tracks the perf issues caused by the content process doing sync IPC to the parent.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey ehsan,

Thanks for organizing these bugs! Is there a concerted effort being started here? Or is this more about gathering bugs together for now?

Comment 2

[(no longer active)]

(In reply to Mike Conley (:mconley) - Getting through review / needinfo backlog from comment #1)
> Hey ehsan,
> 
> Thanks for organizing these bugs! Is there a concerted effort being started
> here?

Not yet, but that may change...  For now I just want a place to look for tracking this stuff.

Comment 3

[(no longer active)]

I'm extending this bug to also track sync IPCs from the UI thread, like bug 1334655.

Comment 4

[(no longer active)]

As a final goal for this bug, I propose writing a patch to the IPDL compiler which would reject all sync IPC messages except for anything that we find where there's no other way to do things that we'll somehow annotate.

That way we won't have to keep fighting this issue forever!

Comment 5

[Marco Castelluccio [:marco]]

(In reply to :Ehsan Akhgari from comment #4)
> As a final goal for this bug, I propose writing a patch to the IPDL compiler
> which would reject all sync IPC messages except for anything that we find
> where there's no other way to do things that we'll somehow annotate.
> 
> That way we won't have to keep fighting this issue forever!

What about doing this now? Adding the annotation to all the current ones and forbidding new sync messages to be created.
This way no new ones can be introduced while the currently existing ones are being fixed.

Comment 6

[(no longer active)]

(In reply to Marco Castelluccio [:marco] from comment #5)
> (In reply to :Ehsan Akhgari from comment #4)
> > As a final goal for this bug, I propose writing a patch to the IPDL compiler
> > which would reject all sync IPC messages except for anything that we find
> > where there's no other way to do things that we'll somehow annotate.
> > 
> > That way we won't have to keep fighting this issue forever!
> 
> What about doing this now? Adding the annotation to all the current ones and
> forbidding new sync messages to be created.
> This way no new ones can be introduced while the currently existing ones are
> being fixed.

That's a fantastic idea!

Comment 7

[Andrew McCreight [:mccr8]]

Here's a list of all the sync messages, I think.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

and for JS side then sendSyncMessage

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I wonder if we could change the name of sendSyncMessage. Could it be sendSlowSyncMessage or something which warns the patch author and reviewer.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #9)
> I wonder if we could change the name of sendSyncMessage. Could it be
> sendSlowSyncMessage or something which warns the patch author and reviewer.

Would it break addons? Otherwise it would be pretty straightforward to search & replace all the files.

Comment 11

[(no longer active)]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #10)
> (In reply to Olli Pettay [:smaug] from comment #9)
> > I wonder if we could change the name of sendSyncMessage. Could it be
> > sendSlowSyncMessage or something which warns the patch author and reviewer.
> 
> Would it break addons? Otherwise it would be pretty straightforward to
> search & replace all the files.

Yes, unfortunately this function is super popular in add-ons. :( 
https://dxr.mozilla.org/addons/search?q=sendSyncMessage&redirect=true

Comment 12

[Mike Conley (:mconley) (:⚙️)]

We could shim it, perhaps.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) from comment #12)
> We could shim it, perhaps.

Although that won't be useful for add-ons that are marked multiprocessCompatible - and these are the ones probably most likely to be using sendSyncMessage. :/ So nevermind.

Comment 14

[Andrew Overholt [:overholt]]

Sorry, should already have marked these meta bugs as P3.