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

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dvander, Assigned: domfarolino)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Assignee: nobody → domfarolino
Posted patch bug1373088.patch (obsolete) — Splinter Review
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
Attachment #8878659 - Flags: review?(milan)
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.
Attachment #8878659 - Flags: review?(milan)
Posted patch bug1373088.patch (obsolete) — Splinter Review
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
Attachment #8878659 - Attachment is obsolete: true
Attachment #8878699 - Flags: review?(milan)
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.)
Attachment #8878699 - Flags: review?(milan) → review+
Addressing comment. Commit message is more complete now, my bad!
Attachment #8878699 - Attachment is obsolete: true
Attachment #8879295 - Flags: review?(milan)
Attachment #8879295 - Flags: review?(milan) → review+
Assignee

Updated

2 years ago
Keywords: checkin-needed
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
Flags: needinfo?(domfarolino)
Keywords: checkin-needed
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.
Assignee

Updated

2 years ago
Flags: needinfo?(domfarolino)
Keywords: checkin-needed

Comment 8

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1102a95be606
Don't use a timeout for compositor process startup when debugging child process. r=milan
Keywords: checkin-needed

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1102a95be606
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.