Race condition setting up permissions at startup

RESOLVED FIXED

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: daleharvey, Assigned: fabrice)

Tracking

unspecified
x86
macOS
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: b2g-desktop-builds)

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

7 years ago
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
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
This issue also fails B2G on Galaxy SII, but not on Otoro.
Reporter

Comment 3

7 years ago
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
Attachment #672401 - Attachment is obsolete: true
Attachment #672403 - Attachment is obsolete: true
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).
Attachment #672406 - Flags: review?(fabrice)
Assignee

Updated

7 years ago
Attachment #672406 - Flags: review?(fabrice) → review?(anygregor)
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.
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?
blocking-basecamp: --- → ?
Can you please confirm that this issue happens on unagi so that we have the correct scope when deciding whether this blocks basecamp.
Flags: needinfo?(poirot.alex)
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.
Attachment #672406 - Flags: review?(anygregor) → review+
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?
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.
Summary: b2g desktop fails to launch with DEBUG gaia → [Desktop Build]b2g desktop fails to launch with DEBUG gaia
Whiteboard: b2g-desktop-builds
Assignee

Comment 14

7 years ago
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).
Attachment #674084 - Flags: review?(anygregor)
Reporter

Comment 15

7 years ago
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
Assignee

Updated

7 years ago
Summary: [Desktop Build]b2g desktop fails to launch with DEBUG gaia → Race condition setting up permissions at startup
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?
Flags: needinfo?(poirot.alex)
Assignee

Comment 17

7 years ago
(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).
Fabrice thinks this is a bad enough user experience to warrant being a blocker.
blocking-basecamp: ? → +
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 on attachment 674084 [details] [diff] [review]
alternate patch

Looks good to me!
Attachment #674084 - Flags: review?(anygregor) → review+
https://hg.mozilla.org/mozilla-central/rev/fb3f0836b834
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.