1656562 - Using Region API in early startup throws error NS_ERROR_NOT_AVAILABLE

Status: RESOLVED FIXED

(Firefox :: Messaging System, defect, P1)

Description

[Punam Dahiya [:pdahiya]]

Using Region API in early startup throws errors NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIPrefBranch.setCharPref] Region.jsm 300

Ideally Region API should fail gracefully with no errors when invoked during early startup. Error can be replicated in Browser Content Toolbox with logging Region.home inside AboutWelcomeChild as below:

ChromeUtils.defineModuleGetter(
this,
"Region",
"resource://gre/modules/Region.jsm"
);

AWWaitForRegionChange() {
log.debug(Region home is ${Region.home});
}

As of now Onboarding flow relies on observing search.region pref directly

https://searchfox.org/mozilla-central/source/browser/components/newtab/aboutwelcome/AboutWelcomeChild.jsm#290

Once this issue is fixed, Ideally we should observe Region change API instead of pref as below:

AWWaitForRegionChange() {
return this.wrapPromise(
new Promise(resolve =>
Services.obs.addObserver(function observer(aSubject, aTopic, aData) {
if (
aData === Region.REGION_UPDATED &&
aTopic === Region.REGION_TOPIC
) {
Services.obs.removeObserver(observer, aTopic);
resolve(Region.home);
}
}, Region.REGION_TOPIC)
)
);
}

Comment 1

[Mark Banner (:standard8)]

How early is about:welcome being started/displayed? Is this displayed instead of about:home/newtab, or is this before any other windows?

As far as I can tell, Region.jsm starts up just fine under normal startup, where I think the search service is the first thing that triggers it. If about:welcome is being loaded earlier, that makes me concerned about the potential performance implications - as I know there's been a lot of work to move loading pages later in the startup and make sure we don't load things too early.

Additionally, if it is being loaded that early, and can't write to prefs, that makes me concerned about why we can't write to prefs - if the pref service isn't available yet, then I'm not sure we should be attempting to start at all.

Not saying we can't try to fix this, but I think we need to understand a bit more about what's happening to make sure it is the right thing to do.

Comment 2

[Punam Dahiya [:pdahiya]]

(In reply to Mark Banner (:standard8) from comment #1)

How early is about:welcome being started/displayed? Is this displayed instead of about:home/newtab, or is this before any other windows?

about:welcome is loaded early during first startup by AboutRedirector using AboutNewTabService. Its displayed instead of about:home/newtab as first welcome screen after user installs Firefox.

https://searchfox.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp#188
https://searchfox.org/mozilla-central/source/browser/components/newtab/AboutNewTabService.jsm#15

This is fairly early in process and I have noticed browser.search.region is empty initially and about:welcome loads before SearchService has updated search.region pref.

As far as I can tell, Region.jsm starts up just fine under normal startup, where I think the search service is the first thing that triggers it. If about:welcome is being loaded earlier, that makes me concerned about the potential performance implications - as I know there's been a lot of work to move loading pages later in the startup and make sure we don't load things too early.

If loading and initializing Region API during early startup has performance implication, may be observing and waiting async for pref to update is right approach here.

Additionally, if it is being loaded that early, and can't write to prefs, that makes me concerned about why we can't write to prefs - if the pref service isn't available yet, then I'm not sure we should be attempting to start at all.

That's correct, it will be good to know cause of failure here and handle it gracefully without error in console, is it because of failure to write to pref due to early startup or may be using API from content process?

Not saying we can't try to fix this, but I think we need to understand a bit more about what's happening to make sure it is the right thing to do.

Comment 3

[Mark Banner (:standard8)]

(In reply to Punam Dahiya [:pdahiya] from comment #2)

(In reply to Mark Banner (:standard8) from comment #1)

Additionally, if it is being loaded that early, and can't write to prefs, that makes me concerned about why we can't write to prefs - if the pref service isn't available yet, then I'm not sure we should be attempting to start at all.

That's correct, it will be good to know cause of failure here and handle it gracefully without error in console, is it because of failure to write to pref due to early startup or may be using API from content process?

Ah that's the missing bit. The pref service isn't able to write from the content process: https://searchfox.org/mozilla-central/rev/03794edd6edcc3fc1e222de966cb27256ce08998/modules/libpref/Preferences.cpp#109

So really, Region.jsm should probably be used from the main process, and have the notifications/values proxied across.

Comment 4

[Dale Harvey (:daleharvey)]

Hey Punam

So I can see in AboutNewTabService its desiged to run in both processes, with some special considerations if its loaded in the parent process, will it be possible to have Region.jsm accessed inside that when in the parent process instead of the content process?

As mentioned Region.jsm isnt really designed to be used in content process, if you can use it in the parent process then we may wat to put a little check in there just to prevent confusion.

Cheers

Comment 5

[Punam Dahiya [:pdahiya]]

Attachment: Bug 1656562 - Use Region API from AboutWelcomeParent

Comment 6

[Punam Dahiya [:pdahiya]]

(In reply to Dale Harvey (:daleharvey) from comment #4)

Hey Punam

So I can see in AboutNewTabService its desiged to run in both processes, with some special considerations if its loaded in the parent process, will it be possible to have Region.jsm accessed inside that when in the parent process instead of the content process?

Tried in attached patch to use Region API from AboutWelcomeParent and NS_ERROR_NOT_AVAILABLE error isn't seen confirming Region API is accessible from parent process in early startup.

As mentioned Region.jsm isnt really designed to be used in content process, if you can use it in the parent process then we may wat to put a little check in there just to prevent confusion.

If using Region API from parent process is recommended and there aren't any explicit performance gains from observing in content process we can land this patch in tree. Thanks!

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:daleharvey, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Dale Harvey (:daleharvey)]

Awesome yeh looking at that patch looks good to me, glad its sorted

Comment 9

[Punam Dahiya [:pdahiya]]

Thanks Dale, updating component of the bug to fix in Messaging System

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3492eea6f5f5
Use Region API from AboutWelcomeParent r=Mardak

Comment 11

[Cosmin Sabou [:CosminS]]

Backed out for browser_ext_themes_ntp_colors_perwindow.js leaks.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=MjnzP_1TRv2JWhQ2lrco4w.0&test_paths=toolkit%2Fcomponents%2Fextensions%2Ftest%2Fbrowser%2Fbrowser.ini&revision=3492eea6f5f53b30dc61347842c4e797df513272

Failure log: https://treeherder.mozilla.org/logviewer?job_id=320794918&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/c512df7e144e3550f4011ef0bc6da0358be2985b

Comment 12

[Punam Dahiya [:pdahiya]]

Hi Rob, You have fixed similar issue https://bugzilla.mozilla.org/show_bug.cgi?id=1584391 in browser_ext_themes_ntp_colors_perwindow.js previously, will be great to get your input on why attached patch in this bug is triggering leaks in debug build. Thanks!

Comment 13

[Rob Wu [:robwu]]

To debug this, I suggest to investigate what could possibly the cause of the leak of a browser window. Is there anything in the AboutWelcomeParent that could result in keeping a reference to the browser window? I would do this:

1. Run test with --verify to see if it reproduces. If you cannot reproduce locally, run a TV test on the try server (perhaps repeatedly). Assuming that you can reproduce, continue as follows.
2. Revert the logic in AboutWelcomeChild.jsm, try to reproduce (I expect that the issue cannot be reproduced).
3. Add a new no-op message listener in AboutWelcomeParent.jsm, and let the implementation in AboutWelcomeChild.jsm await the response from the parent before returning from AWWaitForRegionChange. If the issue is reproduced, then it could be that merely the use of sendQuery and/or wrapPromise is related to the leak.
4. If the previous step is not causing issues, then investigate the current implementation of the message listener.

I see two issues with the current patch:

1. There should be a return after this resolve, to avoid an unnecessary observer: https://hg.mozilla.org/integration/autoland/rev/3492eea6f5f5#l2.34
2. The added observer two lines later may leak if the notification is not triggered.

... but I don't see a super obvious relation between these two implementation issues and the window leak.

Comment 14

[Punam Dahiya [:pdahiya]]

1. There should be a return after this resolve, to avoid an unnecessary observer: https://hg.mozilla.org/integration/autoland/rev/3492eea6f5f5#l2.34
   Thanks Rob , I was able to replicate test failure with local debug build. It looks like this unecessary observer was the issue and adding explicit return after resolve helped fix the test

Comment 15

[Rob Wu [:robwu]]

(In reply to Punam Dahiya [:pdahiya] from comment #14)

1. There should be a return after this resolve, to avoid an unnecessary observer: https://hg.mozilla.org/integration/autoland/rev/3492eea6f5f5#l2.34

Thanks Rob , I was able to replicate test failure with local debug build. It looks like this unecessary observer was the issue and adding explicit return after resolve helped fix the test

This implementation is still somewhat flawed by design. If the observer is never triggered before the window is destroyed, then there is still going to be a leak. To rule out that issue, you can store the observer as a member of the AboutWelcomeParent (if it hasn't been created before) and append the resolve/reject pair to the list. If the observer is triggered, unregister the observer and remove the member, and call all stored resolve functions. If didDestroy is called while the observer hasn't been triggered, unregister the observer and call all reject calls. If it helps, you can use PromiseUtils.jsm for convenience.

Comment 16

[Punam Dahiya [:pdahiya]]

This implementation is still somewhat flawed by design. If the observer is never triggered before the window is destroyed, then there is still going to be a leak. To rule out that issue, you can store the observer as a member of the AboutWelcomeParent (if it hasn't been created before) and append the resolve/reject pair to the list. If the observer is triggered, unregister the observer and remove the member, and call all stored resolve functions. If didDestroy is called while the observer hasn't been triggered, unregister the observer and call all reject calls. If it helps, you can use PromiseUtils.jsm for convenience.

Updated patch with feedback, thanks for optimizing and helping solidify the patch!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb0e384b921a
Use Region API from AboutWelcomeParent r=Mardak,robwu

Comment 18

[Dorel Luca [:dluca]]

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