Closed Bug 1089405 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jlal, Assigned: jlal)

References

Details

Attachments

(5 files, 2 obsolete files)

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.
Attached file full_test.log
This is the full log from the test run
Attached file abnormal2.log
Just the settings part of the logs.
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.
Attached file 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)
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 =/
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.
Attachment #8511719 - Flags: review?(fabrice)
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
Attachment #8511719 - Flags: review?(fabrice)
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?
Flags: needinfo?(fabrice)
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.
Attachment #8511719 - Attachment is obsolete: true
Attachment #8511729 - Flags: review?(fabrice)
Update per your suggestion:

try: https://tbpl.mozilla.org/?tree=Try&rev=a7004bcc6944
Flags: needinfo?(fabrice)
Assignee: nobody → jlal
Status: NEW → ASSIGNED
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.
Attachment #8511729 - Flags: review?(fabrice) → review+
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.
Attachment #8511729 - Attachment is obsolete: true
Attachment #8511735 - Flags: review?(fabrice)
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.
Attachment #8511735 - Flags: review?(fabrice) → review+
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.
Attachment #8511719 - Flags: review+
Attachment #8511719 - Attachment is obsolete: false
Attachment #8511735 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e966f877cbdb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.