Data submission request after shutdown request should not be honored

RESOLVED FIXED in Firefox 21

Status

P1
normal
RESOLVED FIXED
6 years ago
6 days 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
Created attachment 725454 [details] [diff] [review]
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.
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
Last Resolved: 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+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/0039a3b42035
status-firefox20: --- → unaffected
status-firefox21: --- → fixed
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+

Updated

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

Updated

6 days 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.