1373088 - Don't use a timeout for compositor process startup when debugging

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

We have a 5 second timeout for compositor process startup. Milan noted that when using MOZ_DEBUG_CHILD_PROCESS or MOZ_DEBUG_CHILD_PAUSE, we're basically guaranteed the compositor process will be terminated. We should ignore the timeout if these environment variables are set.

Bonus if we can use this bug to rename "layers.gpu-process.timeout_ms" to "layers.gpu-process.startup_timeout_ms", given the work happening in bug 1362166 we'll now have two timeouts.

Comment 1

[Dominic Farolino [:domfarolino]]

Attachment: bug1373088.patch

My first take on this bug. Assuming I've understood it correctly the following patch should be a viable fix + I rename the gfx pref that David mentioned.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0eaa8dda8bcf43c1f8a530573dc7f599a5365dd4

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8878659 [details] [diff] [review]
bug1373088.patch

Review of attachment 8878659 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/ipc/GPUProcessHost.cpp
@@ +59,5 @@
>    if (mLaunchPhase == LaunchPhase::Complete) {
>      return !!mGPUChild;
>    }
>  
> +  #if defined(MOZ_DEBUG_CHILD_PROCESS) || defined(MOZ_DEBUG_CHILD_PAUSE)

Not quite, this is a compile time switch - we need a runtime, based on the environment variable set on the system when we run.  So, something like https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#554

@@ +68,5 @@
>  
>    // Our caller expects the connection to be finished after we return, so we
>    // immediately set up the IPDL actor and fire callbacks. The IO thread will
>    // still dispatch a notification to the main thread - we'll just ignore it.
>    bool result = GeckoChildProcessHost::WaitUntilConnected(timeoutMs);

We still want to get here, even when "debug child process" is set.  We just don't want the timeout.  So, you would want to call WaitUntilConnected with a 0 (or a negative number) to indicate "do not time out" when one of the environment variables in question are set.

Comment 3

[Dominic Farolino [:domfarolino]]

Attachment: bug1373088.patch

Gotcha. Makes sense now. I opted to set timeoutMs = 0 in the event that one of these vars is set - in my latest patch.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03b8582f23cae255a2efb8e21c76592f2a51caec

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8878699 [details] [diff] [review]
bug1373088.patch

Review of attachment 8878699 [details] [diff] [review]:
-----------------------------------------------------------------

Make sure the comment is complete (e.g., "Don't use a timeout for compositor process startup when debugging child process startup" or something like that.)

Comment 5

[Dominic Farolino [:domfarolino]]

Attachment: bug1373088.patch

Addressing comment. Commit message is more complete now, my bad!

Comment 6

[Carsten Book [:Tomcat]]

has problems to apply:

renamed 1373088 -> bug1373088.patch
applying bug1373088.patch
patching file gfx/thebes/gfxPrefs.h
Hunk #1 FAILED at 545
1 out of 1 hunks FAILED -- saving rejects to file gfx/thebes/gfxPrefs.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory

Comment 7

[Dominic Farolino [:domfarolino]]

Hmm I've rebased this patch onto the tip of mozilla/central and it applied cleanly, and I got a working build from it. I'm guessing we should just try and re-merge this as it may go in now? I'll re-add the checkin-needed keyword.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1102a95be606
Don't use a timeout for compositor process startup when debugging child process. r=milan

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1102a95be606