Ease building gaia profile compatible with the simulator

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 821151 [details] [review]
Pull request 13041
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
(Assignee)

Comment 4

5 years ago
Comment on attachment 821151 [details] [review]
Pull request 13041

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing 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?
(Assignee)

Comment 5

5 years ago
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:
https://github.com/mozilla-b2g/gaia/commit/61f7114911b8dd5e8e593a90021a909e6aced65c
Attachment #821151 - Flags: approval-gaia-v1.2?
(Assignee)

Comment 6

5 years ago
Created attachment 825292 [details] [review]
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)
(Assignee)

Updated

5 years ago
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.

master-revert: https://github.com/mozilla-b2g/gaia/commit/083e4e6d362ef2e56dfce5ec855c4f79e72699ac
(Assignee)

Comment 9

5 years ago
Created attachment 828028 [details] [review]
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.
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 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: https://github.com/mozilla-b2g/gaia/commit/fa29c2b37b44247a3d1e133924346b22ead41cd3
Status: RESOLVED → REOPENED
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!
(Assignee)

Comment 16

5 years ago
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\system.gaiamobile.org\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:
  http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#74

This is new or new usages of msgmgr have been added... I'll try to tune Firefox codebase to get rid of this exception.
(Assignee)

Comment 17

5 years ago
Created attachment 8334466 [details] [diff] [review]
Ensure shipping nsISystemMessagesInternal interface in Firefox r=jonas
(Assignee)

Comment 18

5 years ago
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: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=938960&attachment=832699

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)
(Assignee)

Comment 22

5 years ago
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)
(Assignee)

Comment 23

5 years ago
Comment on attachment 828028 [details] [review]
Pull request 13884

>https://github.com/mozilla-b2g/gaia/pull/13884
Attachment #828028 - Attachment description: Pull request 13429 → Pull request 13884
Attachment #828028 - Flags: review?(kgrandon)
(Assignee)

Comment 24

5 years ago
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: https://github.com/mozilla-b2g/gaia/commit/dc6d646ca74eb4983e6eaf645fdc477717cfa25f
Attachment #828028 - Flags: review?(kgrandon) → review+
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 26

5 years ago
Created attachment 8335621 [details] [review]
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.