Data submission request after shutdown request should not be honored

RESOLVED FIXED in Firefox 21

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
9 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
Firefox 22

Firefox Tracking Flags

(firefox20 unaffected, firefox21 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

6 years ago
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!
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.
Priority: -- → P1
Assignee

Updated

6 years ago
Assignee: nobody → gps
Assignee

Comment 2

6 years ago
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.
Attachment #725454 - Flags: review?(rnewman)
Assignee

Updated

6 years ago
Status: NEW → ASSIGNED
Attachment #725454 - Flags: review?(rnewman) → review+
Assignee

Comment 4

6 years ago
https://hg.mozilla.org/mozilla-central/rev/e0d0c6a85a4a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla22
Assignee

Comment 5

6 years ago
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.
Attachment #725454 - Flags: approval-mozilla-aurora?
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 :)
Attachment #725454 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Considering this is a Beta blocker, is there something I can do to confirm this is working as expected?
Assignee

Comment 9

6 years ago
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.
(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?
Flags: in-testsuite?
Assignee

Comment 11

6 years ago
Yup.
Flags: in-testsuite? → in-testsuite+
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22

Updated

9 months ago
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.