845935 - Data submission request after shutdown request should not be honored

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect, P1)

Description

[Gregory Szorc [:gps]]

From log in bug 845842:

1361986826492	Services.HealthReport.HealthReporter	INFO	Request to shut down.
1361986826492	Services.HealthReport.HealthReporter	WARN	Collector is in progress of initializing. Waiting to finish.
1361986876724	Services.DataReporting.Policy	INFO	Requesting data submission. Will expire at Wed Feb 27 2013 12:51:16 GMT-0500 (EST)
1361986876728	Services.Metrics.Collector	INFO	Initializing provider with storage: org.mozilla.appInfo

While it should be a small window, we shouldn't honor submission requests once shutdown has been initiated!

Comment 1

[:Irving Reid (No longer working on Firefox)]

Is this going to be sensitive to the order we run shutdown handlers? Is it OK for other modules to request data submissions while they are shutting down? If so, FHR will need to wait for everything else to shut down before it closes the gate.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Check initialized state, v1

This should do it.

I think the next time we significantly refactor this file we should use distinct types to model the different service states instead of tracking state within one type. e.g.

  let hr = getHealthReporter(...) -> UninitializedHealthReporter
  hr.init().then(function onInit(reporter) { ... }) -> InitializedHealthReporter

Something to think about.

Comment 3

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/e0d0c6a85a4a

Comment 4

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/e0d0c6a85a4a

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 725454 [details] [diff] [review]
Check initialized state, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR initial landing.
User impact if declined: FHR could start doing work during shutdown. This would delay shutdown and create a bad user experience, especially on slow machines.
Testing completed (on m-c, etc.): It's been on m-c for a few days. We added tests.
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: None

This is a P1 FHR bug, which means it's a beta blocker. If we don't uplift this to 21, it may delay FHR by a release.

Comment 6

[bhavana bajaj [:bajaj]]

Comment on attachment 725454 [details] [diff] [review]
Check initialized state, v1

low risk, P1 requirement for FHR and has baked on m-c for a few days.Approving for uplift .

Thanks for the adding the tests here :)

Comment 7

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0039a3b42035

Comment 8

[u279076]

Considering this is a Beta blocker, is there something I can do to confirm this is working as expected?

Comment 9

[Gregory Szorc [:gps]]

I think it would be very difficult to verify this manually. We have test coverage in the patch which I think is sufficient.

If this bug were causing issues in the wild, we'd hear reports of shutdown hangs.

Comment 10

[u279076]

(In reply to Gregory Szorc [:gps] from comment #9)
> I think it would be very difficult to verify this manually. We have test
> coverage in the patch which I think is sufficient.
> 
> If this bug were causing issues in the wild, we'd hear reports of shutdown
> hangs.

Okay thanks Gregory. Should this be marked in-testsuite?

Comment 11

[Gregory Szorc [:gps]]

Yup.