Closed Bug 930104 Opened 9 years ago Closed 9 years ago

Ease building gaia profile compatible with the simulator


(Firefox OS Graveyard :: Gaia, defect)

Not set


(Not tracked)



(Reporter: ochameau, Assigned: ochameau)




(3 files, 2 obsolete files)

While looking at bug 929061, I've identified some bits in gaia build system that prevent from easily building a gaia profile that would work with the app manager simulator.

The biggest issue is the trick used to make the app permission to work in Firefox. We are setting the system app local Id to 0 in order to magically trick firefox permissions. That trick has the opposite effect on the simulator.

I found a way to make permissions work on firefox without this. The good artefact of this is that is will also allow to make permissions work for any app in any tab. Today, if you open an app in a tab, its permissions won't work. Only the system app (and all apps being loaded inside the system app) will have valid permissions.
Attached file Pull request 13041 (obsolete) —
Attachment #821151 - Flags: review?(21)
Comment on attachment 821151 [details] [review]
Pull request 13041

That's a cool hack. thanks!
Attachment #821151 - Flags: review?(21) → review+
Duplicate of this bug: 896526
Comment on attachment 821151 [details] [review]
Pull request 13041

NOTE: Please see to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): none
[User impact] if declined: no user impact, will ease making the simulator work with 1.2 branch and improve gaia dev quality of life on Firefox.
[Testing completed]: baked on master for a bit.
[Risk to taking this patch] (and alternatives if risky): very limited, only build script changes, only about desktop/simulator mode.
[String changes made]: none
Attachment #821151 - Flags: approval-gaia-v1.2?
Comment on attachment 821151 [details] [review]
Pull request 13041

Oh I was convinced I clicked on the github green button, but I didn't.
Let the patch land first on master:
Attachment #821151 - Flags: approval-gaia-v1.2?
Attached file Pull request 13258
This patch introduces a SIMULATOR=1 env variable that mainly prevents installing firefox addons when building a desktop profile.
I could have tweaked DESKTOP=1 behavior, but I prefered not to as it might impact tests. 
At the end SIMULATOR=1 allows to have an optimized build of gaia for the simulator usage. I should be able to uplift part of the customization being done in simulator to gaia repo behind this flag.
Attachment #825292 - Flags: review?(21)
Attachment #825292 - Flags: review?(21) → review?(yurenju.mozilla)
Comment on attachment 825292 [details] [review]
Pull request 13258

looks good!
Attachment #825292 - Flags: review?(yurenju.mozilla) → review+
Comment on attachment 821151 [details] [review]
Pull request 13041

Sorry guys, I had to revert this patch. It was causing the unit tests not to work unless you built a profile without the desktop helper. I can help investigate this, but it's not obvious to me what's going on here.

Attached file Pull request 13884
I've addressed the test-agent issue by preventing setting the valid app id for test-agent iframes. That's kind of bad, but test-agent expects to load apps into iframes without any permission.
Attachment #821151 - Attachment is obsolete: true
Attachment #828028 - Flags: review?(yurenju.mozilla)
Comment on attachment 828028 [details] [review]
Pull request 13884

I can't fully understand this patch but looks good, f=yurenju, I thought vivien is better for reviewing this pr.
Attachment #828028 - Flags: review?(yurenju.mozilla)
Attachment #828028 - Flags: review?(21)
Attachment #828028 - Flags: feedback+
and travis is red state, please rebase and update this pull request again.
Closed: 9 years ago
Resolution: --- → FIXED
I'm really sorry, but we had to revert again. Got a few reports of people being unable to open gaia in Firefox after this patch. I'm able to reproduce it by applying the patch, deleting the profile-debug folder, then rebuilding.

Backed out:
Resolution: FIXED → ---
Depends on: 940183
Note: This is not blocked on landing bug 940183, but I wanted to add the dependency so we could illuminate what kind of issues we will solve with the testing. Thanks!
Sorry about that, I forgot to test again after the rebase for travis...

This patch faces a "regression" on Firefox, where Webapps.jsm throws on startup and isn't able to correctly register the system app in the webapps registry:

DOMApplicationRegistry: Could not parse JSON: c:\Users\Alex\Desktop\mozilla\gaia\profile-debug\webapps\\manifest.webapp [Exception... "Component returned failure code: 0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.getService]"  nsresult: "0x80570018 (NS_ERROR_XPC_BAD_IID)"  location: "JS frame :: resource://gre/modules/Webapps.jsm :: <TOP_LEVEL> :: line 69"  data: no]
msgmgr is not defined

That's because Ci.nsISystemMessagesInternal is undefined on Firefox:

This is new or new usages of msgmgr have been added... I'll try to tune Firefox codebase to get rid of this exception.
Comment on attachment 8334466 [details] [diff] [review]
Ensure shipping nsISystemMessagesInternal interface in Firefox r=jonas

In bug 921637, we missed the addition of the xpt file for system message manager interfaces. So that we do have all *.js files like SystemMessageManager.js and ActivityWrappers.js, but various code in Webapps.jsm throws as nsISystemMessagesInternal is undefined on Firefox.

These exception prevents from registering correctly some apps in the webapps registry. Then the faulty apps don't have correct permissions.
Attachment #8334466 - Flags: review?(jonas)
Hi Alex!

Very sorry about that - it seems that I may have indeed caused it to fail. Please see bug 938960 as I think the patch should go in today's nightly.
Comment on attachment 8334466 [details] [diff] [review]
Ensure shipping nsISystemMessagesInternal interface in Firefox r=jonas

Obsoleting as this just landed:

If I'm wrong, please un-obsolete. I'll test the gaia pieces again as well.
Attachment #8334466 - Attachment is obsolete: true
Attachment #8334466 - Flags: review?(jonas)
Hi Alex,

I tried testing your patch to re-land, and I am receiving the following error. I haven't dug into this too far yet:

[Exception... "'[JavaScript Error: "interAppCommService is not defined" {file: "resource://gre/modules/Webapps.jsm" line: 547}]' when calling method: [nsIRequestObserver::onStopRequest]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
Flags: needinfo?(poirot.alex)
Yes, webapps registry still fails to register the system app because of this yet another component we miss on Firefox. I don't think it is sustainable to keep adding unecessary stuff by default into Firefox.
Also, I have a typo in my patch: startWith instead of startsWith.

But there is something I don't understand: why was that working correctly few days ago and sudently fails for various reasons...!
Flags: needinfo?(poirot.alex)
Comment on attachment 828028 [details] [review]
Pull request 13884

Attachment #828028 - Attachment description: Pull request 13429 → Pull request 13884
Attachment #828028 - Flags: review?(kgrandon)
Kevin, I just pushed a new pull request, just with the startsWith typo fix, it appears to work fine.
I thought it was broken because of this patch, but actually, I was facing a regression in Firefox happening only on Windows (bug 910885 comment 6).

Could you confirm that this patch works fine?
Comment on attachment 828028 [details] [review]
Pull request 13884

Tested, and everything seems to work fine.

Landed in master:
Attachment #828028 - Flags: review?(kgrandon) → review+
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Attached file Pull request 13889
Hopefully last hotfix to fix all issues with the localId=0 hack removal.
Attachment #8335621 - Flags: review?(kgrandon)
You need to log in before you can comment on or make changes to this bug.