830809 - Remove 'shutdown start' label with eConsiderQuit

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

The labels were incorrectly showing up when we were calling with eConsiderQuit. Need to move the label to when we've decided to shutdown.

Comment 1

[(no longer active)]

Comment on attachment 702344 [details] [diff] [review]
patch

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

::: toolkit/components/startup/nsAppStartup.cpp
@@ +361,5 @@
>    }
>  
>    nsCOMPtr<nsIObserverService> obsService;
>    if (ferocity == eAttemptQuit || ferocity == eForceQuit) {
> +    SAMPLE_MARKER("Shutdown start");

If you're interested in the point of no return, that's further down this function, where we set mShuttingDown = true.  The new place for the marker can also be too early since we may be unable to close some of our windows.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch v2

Alright this means we could potentially miss quite a bit of work:
1) We can either assume the user will look at the samples a bit before this marker
2) Or we could have a shutdown cancel marker as well.

This patch does 1 which I think is ok.

Comment 3

[Vladan Djeric (:vladan)]

Rafael, does the bug addressed in this patch imply we might be recording the shutdown timestamp multiple times during a session?

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Vladan Djeric (:vladan) from comment #3)
> Rafael, does the bug addressed in this patch imply we might be recording the
> shutdown timestamp multiple times during a session?

Yes, but "recording" in here is just setting a global variable. 

void
RecordShutdownStartTimeStamp() {
  if (!Telemetry::CanRecord())
    return;

  gRecordedShutdownStartTime = TimeStamp::Now();

  GetShutdownTimeFileName();
}

Which should be a nop. In any case, we would probably move it to the earliest point we know firefox will be shutdown and make sure it is called only once.

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

When do we call with eConsiderQuit? I have done to try pushes to check this:

Just add an assert:
https://tbpl.mozilla.org/?tree=Try&rev=bbd6c5d33b88

Add the assert and move:
 https://tbpl.mozilla.org/?tree=Try&rev=062eb1142bb5

I will probably send the second one (rebased on top of this) for review if it is green.

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I reported bug 831303 to fix the mozilla::RecordShutdownStartTimeStamp call.

Comment 7

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/712eca11a04e

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/712eca11a04e