1053733 - [Settings] SettingManager is unable to receive message sent from SettingsChangeNotifier if SettingManager is initialized before SettingsChangeNotifier

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Edgar Chen [:edgar] (PTO until 02/09)]

I meet this issue when working on bug 843452, please see bug 843452 comment #183.
It is 100% reproducible on test_webapps_actor.html mochitest test with applying patches of bug 843452.

SettingsManager sends Settings:RegisterForMessages in it's init process, but SettingsChangeNotifier may not yet be initialized and SettingsChangeNotifier lost this register message.

Sometime it can be recovered if someone calls AddObserver. But if AddObserver is never be called, SettingsManager is unable to receive messages from SettingsChangeNotifier.

Comment 1

[Edgar Chen [:edgar] (PTO until 02/09)]

(In reply to Edgar Chen [:edgar][:echen] from comment #0)
> I meet this issue when working on bug 843452, please see bug 843452 comment
> #183.
> It is 100% reproducible on test_webapps_actor.html mochitest test with
> applying patches of bug 843452.
> 
> SettingsManager sends Settings:RegisterForMessages in it's init process, but
> SettingsChangeNotifier may not yet be initialized and SettingsChangeNotifier
> lost this register message.
> 
> Sometime it can be recovered if someone calls AddObserver. But if
> AddObserver is never be called, SettingsManager is unable to receive
> messages from SettingsChangeNotifier.

Or addObserver is called before SettingsChangeNotifier is ready.

Comment 2

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Patch, v1

Comment 3

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8472986 [details] [diff] [review]
Patch, v1

We don't know when SettingChangeNotifier is ready for receiving register event. It seems a new message is needed for notifying SettingChangeNotifier is ready. And SettingManager can send register message again when receiving it.

Hi Gregor, may I have your feedback for this patch? Thank you

Comment 4

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Set feature-b2g as 2.1 because it blocks bug 843452

Comment 5

[Gregor Wagner [:gwagner]]

We are about to land bug 846200. Lets see if this is still necessary after we land.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, we rip out SettingsChangeNotifier in bug 900551 (also landing ASAP), and it should now be handled by SettingsRequestManager. We bring up SettingsRequestManager in shell.js, but I'm not sure what the timing on that will be versus where you're registering for listeners.

Comment 7

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8472986 [details] [diff] [review]
Patch, v1

(In reply to Gregor Wagner [:gwagner] from comment #5)
> We are about to land bug 846200. Lets see if this is still necessary after
> we land.

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #6)
> Yeah, we rip out SettingsChangeNotifier in bug 900551 (also landing ASAP),
> and it should now be handled by SettingsRequestManager. We bring up
> SettingsRequestManager in shell.js, but I'm not sure what the timing on that
> will be versus where you're registering for listeners.

Thanks for this information, Kyle and Gregor. :)

I will try it again after bug 900551 and bug 846200 is landed.

Comment 8

[Edgar Chen [:edgar] (PTO until 02/09)]

Hmm, I try to test this with applying patches of bug 846200 and bug 900551, but (In reply to Edgar Chen [:edgar][:echen] from comment #7)
> Comment on attachment 8472986 [details] [diff] [review]
> Patch, v1
> 
> (In reply to Gregor Wagner [:gwagner] from comment #5)
> > We are about to land bug 846200. Lets see if this is still necessary after
> > we land.
> 
> (In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #6)
> > Yeah, we rip out SettingsChangeNotifier in bug 900551 (also landing ASAP),
> > and it should now be handled by SettingsRequestManager. We bring up
> > SettingsRequestManager in shell.js, but I'm not sure what the timing on that
> > will be versus where you're registering for listeners.
> 
> Thanks for this information, Kyle and Gregor. :)
> 
> I will try it again after bug 900551 and bug 846200 is landed.

Just for your information.
I did not meet this issue any more after applying the patches of bug 846200 and bug 900551 [1]. Will try it again after they are ready in m-c.

[1] https://hg.mozilla.org/integration/b2g-inbound/rev/82bed38ff50a

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hey Kyle,
Can this patch be taken prior to bug 900551?
This one blocks bug 843452 and bug 889737.

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, go ahead and land. We're hitting really bad problems with marionette webapi tests, so bug 900551 is blocked 'til we get those fixed. Hoping it's tomorrow, but don't want to hold up everyone waiting on that.

Comment 11

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8472986 [details] [diff] [review]
Patch, v1

Hi Kyle, could you help to review this? Thank you :)

Comment 12

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Patch, v1, r=qdot

Just carry r=qdot in comment title.

Thank you, Kyle.

Comment 13

[Kai-Chih Hu [:khu]]

Confirmed with EM/EPM, and this can be landed before FL.

Comment 14

[Edgar Chen [:edgar] (PTO until 02/09)]

Try result looks good: https://tbpl.mozilla.org/?tree=Try&rev=0182a4ec05c3

Comment 15

[Edgar Chen [:edgar] (PTO until 02/09)]

https://hg.mozilla.org/integration/b2g-inbound/rev/7b8ac88b0cf1

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b8ac88b0cf1