1089405 - [Settings] CreateLock not always received by SettingsRequestManager.jsm, locks not resolved

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[James Lal [:lightsofapollo]]

Note that in the constructor we send this message.. I am likely missing some context here but it seems impossible that these messages are getting dropped (unless the window is destroyed somehow and I am not seeing it ?)

The symptom here is the caller never gets a response to their locks transaction (effectively freezing the caller). I think we should do the following:

 - figure out the race
 - add guards to send an error to SettingsManger.js with the given ID so even if the parent does not know about it we can send some type of error to content.

Comment 1

[James Lal [:lightsofapollo]]

Attachment: full_test.log

This is the full log from the test run

Comment 2

[James Lal [:lightsofapollo]]

Attachment: abnormal2.log

Just the settings part of the logs.

Comment 3

[James Lal [:lightsofapollo]]

Full context here 831010... tldr; This prevents shell.js from starting the system app correctly (it waits for the settings db) which in turn (I believe) prevents marionette-content-listener from starting correctly.

Comment 4

[James Lal [:lightsofapollo]]

Attachment: lock.log

Here is a shortened log... It looks like cpmm is sending/receiving the messages correctly for CreateLock but they get lost somewhere in JS.

([ReceiveMessage-start][ReceiveMessage-end] occur here https://github.com/lightsofapollo/gecko-dev/blob/debug-marionette-moar/content/base/src/nsFrameMessageManager.cpp#L934-L1094)

Comment 5

[James Lal [:lightsofapollo]]

Attachment: race_condition_for_init.log

This is a pure race its easy to see now that I added more logs... Search for  SettingsRequestManager: init and you will see that there are a bunch of calls to CreateLock _before_ we start listening for them =/

Comment 6

[James Lal [:lightsofapollo]]

Attachment: Move SettingsRequestManager load to near top of b2g boot process

SettingsRequestManager is responsible for bootstrapping the settings api
without this near the top of the boot process any consumers before race
to aquire locks/get settings.

Comment 7

[James Lal [:lightsofapollo]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8f53e37bbd3f

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 8511719 [details] [diff] [review]
Move SettingsRequestManager load to near top of b2g boot process

Review of attachment 8511719 [details] [diff] [review]:
-----------------------------------------------------------------

There's an even better place if you want to make sure that your jsm is loaded as early at possible. Here: https://mxr.mozilla.org/mozilla-central/source/b2g/components/ProcessGlobal.js#148

Comment 9

[James Lal [:lightsofapollo]]

I would rather keep the settings logic in setting.js (it's a somewhat logical place to put it)... 

Ideally though I would like to move all the implicit startup logic/dependencies (http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#7) outside of shelljs/settings.js but I would like to do it in one shot.

What do you think?

Comment 10

[James Lal [:lightsofapollo]]

Attachment: Move SettingsRequestManager load to near top of b2g boot process

SettingsRequestManager is responsible for bootstrapping the settings api
without this near the top of the boot process any consumers before race
to aquire locks/get settings.

Comment 11

[James Lal [:lightsofapollo]]

Update per your suggestion:

try: https://tbpl.mozilla.org/?tree=Try&rev=a7004bcc6944

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 8511729 [details] [diff] [review]
Move SettingsRequestManager load to near top of b2g boot process

Review of attachment 8511729 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nit fixed.

::: b2g/components/ProcessGlobal.js
@@ +147,5 @@
>          });
>  
> +        // In the parent process bootstrap components that need to load as early
> +        // as possible.
> +        Cu.import('resource://gre/modules/SettingsRequestManager.jsm');

nit: this file uses double quotes for strings.

Comment 13

[James Lal [:lightsofapollo]]

Attachment: Move SettingsRequestManager load to near top of b2g boot process

SettingsRequestManager is responsible for bootstrapping the settings api
without this near the top of the boot process any consumers before race
to aquire locks/get settings.

Comment 14

[James Lal [:lightsofapollo]]

Comment on attachment 8511735 [details] [diff] [review]
Move SettingsRequestManager load to near top of b2g boot process

carrying r+ forward changed the quotes... waiting for try run before landing.

Comment 15

[James Lal [:lightsofapollo]]

Moving import to ProcessGlobal seems to make IDB unhappy (it breaks b2g boot :|) I suspect this is too far up in boot :) ? Landing this in settings.js improves a huge chunk of test stability bugs (and probably a "my phone won't boot sometimes) bug... I would like to land it as it was.

Comment 16

[Gregor Wagner [:gwagner]]

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

Comment 17

[Carsten Book [:Tomcat]]

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