801355 - Race condition setting up permissions at startup

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Dale Harvey (:daleharvey)]

Build
  Gecko: 93486bc92514b553b7d1a3aa14f7c966fafe61c3
  Gaia: d0badbe021fe141f45ae3fc213a6264924e023f7

Doing a fresh complete build with the DEBUG=1 flag used to build gaia the homescreen fails to launch

NS_ERROR_FAILURE: Denied
 ....Webapps.json Line 667

Seems to be the problem, building without the debug flag works

This appears to happen on device builds as well, whether or not gaia was built with DEBUG

Comment 1

[Pavel Franc - Mozilla.cz]

Debug profile is not working for me either.

In jsconsole I can see the same error:

Error: NS_ERROR_FAILURE: Denied
Source File: .../components/Webapps.js   Line:667

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

This issue also fails B2G on Galaxy SII, but not on Otoro.

Comment 3

[Dale Harvey (:daleharvey)]

Hey Fabrice / Mounir

Added you 2 as this seems in your area and it looks like it is still affecting a ton of people.

I plan on looking into it after I get these patches fixed up if nobody else does before me

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 801355: Ensure Permissions JSM is loaded when its companion XPCOM module is used.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 801355: Ensure Permissions JSM is loaded when its companion XPCOM module is used.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 801355: Ensure Permissions JSM is loaded when its companion XPCOM module is used.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 672406 [details] [diff] [review]
Bug 801355: Ensure Permissions JSM is loaded when its companion XPCOM module is used.

Here is a hacky way to fix that.

The original issue is a race condition where PermissionSettings.js (the xpcom) send messages whereas PermissionSettings.jsm (the jsm) isn't loaded yet.

I don't really know who is loading Webapps.jsm which loads PermissionsInstaller and use the xpcom.
I tried to move Cu.import(PermissionsSettings.jsm) on top of shell.js without success (intermittent failure). Nor loading it in Webapps.jsm (DOMApplicationRegistry is undefined exception).

Comment 8

[Les Orchard [:lorchard]]

FWIW, if only for confirmation: The attached patch and a self-built b2g desktop fixed my issue. I was getting a black screen + white "mozilla" in the corner, and the error in comment #0 in my jsconsole.

Comment 9

[Alexandre Poirot [:ochameau]]

Requesting blocking-basecamp as it doesn't seem to be happening only with DEBUG=1 gaia builds. It looks like I can reproduce this race condition on unagi. And lorchard seems to confirm that it happens outside of DEBUG=1 scope too.

I haven't verified either, but desktop build seems broken too.
As it is a race condition it can easily explain why it happens on galaxy s2 and not on otoro, as fast device will easily trigger that error.

anygregor: can you forward this review if you don't have time reviewing it?

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Can you please confirm that this issue happens on unagi so that we have the correct scope when deciding whether this blocks basecamp.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 672406 [details] [diff] [review]
Bug 801355: Ensure Permissions JSM is loaded when its companion XPCOM module is used.

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

The fix seems simple and good enough for me. This race is really painful and can happen on every devices depending on the current load of the device. This also breaks Gaia in FF desktop and in the desktop build.

Comment 12

[Gregor Wagner [:gwagner]]

We also load this module in another place and loading it twice results in an error. Did you make sure that doesn't result in an exception when you load it the 2nd time?

Comment 13

[Mounir Lamouri (:mounir)]

Alexandre, you do not *need* to block basecamp in order to push this to m-a. You can also simply push the patch to m-c and then ask for m-a approval.

Comment 14

[[:fabrice] Fabrice Desré]

Attachment: alternate patch

Here's another approach to the problem. I didn't like too much Alex's patch which is loading a parent side jsm in a DOM facing component that is expected to be loaded in content processes (we've been bitten by that a couple of times already!)

Alex's patch works because the PermissionInstaller.jsm was itself loading the content side xpcom component in the parent process - my bad for not catching that during review. So I removed this from PermissionInstaller.jsm and make it use PermissionSettings.jsm directly. Everything works in my testing, but I'd like to have as many people trying that as possible before we eventually land, including various devices and configurations (DEBUG on desktop, user and eng variants on device).

Comment 15

[Dale Harvey (:daleharvey)]

I have tried it on b2g desktop build both DEBUG and normal make, and on the otoro device with ./flash.sh gaia and make production, all WFM

Comment 16

[Alexandre Poirot [:ochameau]]

WFM: unlock and use browser works without JS error.
On desktop DEBUG=0,1.
Device builds eng and user on unagi.

I'm just unsure if doing VARIANT=user ./build.sh && ./flash.sh on an existing build with default (VARIANT=eng) was enough or should have had clobber my build before using another variant?

Comment 17

[[:fabrice] Fabrice Desré]

(In reply to Alexandre Poirot (:ochameau) from comment #16)
> WFM: unlock and use browser works without JS error.
> On desktop DEBUG=0,1.
> Device builds eng and user on unagi.
> 
> I'm just unsure if doing VARIANT=user ./build.sh && ./flash.sh on an
> existing build with default (VARIANT=eng) was enough or should have had
> clobber my build before using another variant?

I usually clobber the out/ directory when switching from user to eng (no need to clobber gecko).

Comment 18

[Andrew Overholt [:overholt]]

Fabrice thinks this is a bad enough user experience to warrant being a blocker.

Comment 19

[Les Orchard [:lorchard]]

This new patch works for me, too - on b2g desktop, from a clone of m-c updated 5 min ago. (Just to follow up on comment #8)

Comment 20

[Gregor Wagner [:gwagner]]

Comment on attachment 674084 [details] [diff] [review]
alternate patch

Looks good to me!

Comment 21

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3f0836b834

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f839f46d8286