1493041 - Topstories feed uninit can remove idle-daily observer before init has a chance to add it

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Scott [:thecount] Downe]

To test, on nightly.
1. Run mozilla central with --temp-profile
2. Should be an error in the console about trying to remove an observer before it's added. "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]"

The issue is caused by me trying to fix a previous race condition, where I added some awaits. After these awaits, it then adds the observer. In previous code it wouldn't await and thus the observer would be added right away. If the awaits delay the add observer, it makes calling uninit dangerous. Adding the observer before the awaits makes any uninit calls safe.

I don't see a reason why we cannot just move the add observer line up to before the awaits, to work essentially how it did before, and add a test to ensure the fire order.

Comment 1

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4439

Comment 3

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/c58aebf7ec5f6c74118ff76a59380bb158dfa504
Fixes bug 1493041 - Topstories feed uninit can remove idle-daily observer before init has a chance to add it (#4439)

Comment 4

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/95f8e6265f24

Comment 5

[Scott [:thecount] Downe]

Attachment: bug 1493041 - Topstories feed uninit can remove idle-daily observer before init has a chance to add it

Comment 6

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4468

Comment 7

[Scott [:thecount] Downe]

Comment on attachment 9014470 [details]
bug 1493041 - Topstories feed uninit can remove idle-daily observer before init has a chance to add it

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1479458

User impact if declined: Uncaught js error thrown

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: 1. Run mozilla central with --temp-profile
2. Should be an error in the console about trying to remove an observer before it's added. "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]"

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): It's moving one line, it has tests, it's on nightly already, while I don't think it's officially been verified, I have no longer seen the error on nightly in my every day usage and testing. Initially caused from changes in 1479458.

String changes made/needed: None

Comment 8

[Marius Coman [:mcoman], Ecosystem QA]

I have not managed to reproduce the initial issue on a "Firefox Nightly" build from 09/20/2018 (64.0a1 - Build ID 20180920100522), however I have encountered a similar error, but I don't know if it is related, more exactly the "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIInterfaceRequestor.getInterface]" one.

Also, do we need to test this using a local build? I'm asking this because I've used an official "Nightly" release.

Comment 9

[Pascal Chevrel:pascalc]

Comment on attachment 9014470 [details]
bug 1493041 - Topstories feed uninit can remove idle-daily observer before init has a chance to add it

Minimal patch for a regression in 63, uplift approved for 63 beta 13, thanks.

Comment 10

[Scott [:thecount] Downe]

My bad, this initial bug was filed when nightly was still broken, and the steps to test said "To test, on nightly." which isn't the case for the uplift because the initial bug we're uplifting is fixed on nightly.

So to test, try it on beta. It doesn't need to be a local build, nightly and beta builds should adequately show the issue (beta) and the fix (nightly).

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/df28f33c9930

Comment 12

[Marius Coman [:mcoman], Ecosystem QA]

I have verified that this issue is no longer reproducible with the latest version of "Firefox Nightly" (64.0a1 - Build ID 20181004224156) installed, on Windows 10 x64, Arch Linux and Mac 10.13.3. It was necessary to use a VPN in order to reproduce the initial issue, but now in the latest Nightly build, I can confirm that the "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]" error is no longer displayed in the browser console.

Comment 13

[Marius Coman [:mcoman], Ecosystem QA]

I have verified that this issue is no longer reproducible with the latest version of "Firefox Beta" (63.0b13 - Build ID 20181008155858) installed, on Windows 10 x64, Arch Linux and Mac 10.13.3. Now the "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]" error is no longer displayed in the browser console.