Closed
Bug 930104
Opened 11 years ago
Closed 11 years ago
Ease building gaia profile compatible with the simulator
Categories
(Firefox OS Graveyard :: Gaia, defect)
Firefox OS Graveyard
Gaia
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(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.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #821151 -
Flags: review?(21)
Comment 2•11 years ago
|
||
Comment on attachment 821151 [details] [review]
Pull request 13041
That's a cool hack. thanks!
Attachment #821151 -
Flags: review?(21) → review+
Assignee | ||
Comment 4•11 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•11 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•11 years ago
|
||
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•11 years ago
|
Attachment #825292 -
Flags: review?(21) → review?(yurenju.mozilla)
Comment 7•11 years ago
|
||
Comment on attachment 825292 [details] [review]
Pull request 13258
looks good!
Attachment #825292 -
Flags: review?(yurenju.mozilla) → review+
Comment 8•11 years ago
|
||
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•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 825292 [details] [review]
Pull request 13258
Landed on master:
https://github.com/mozilla-b2g/gaia/commit/4e60f8bbab844c4a63f1cfb4be46cb264f9626c8
Comment 11•11 years ago
|
||
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+
Comment 12•11 years ago
|
||
and travis is red state, please rebase and update this pull request again.
Attachment #828028 -
Flags: review?(21) → review+
Assignee | ||
Comment 13•11 years ago
|
||
Comment on attachment 828028 [details] [review]
Pull request 13884
Landed on master:
https://github.com/mozilla-b2g/gaia/commit/5d2ce56dbfbba702a487cd17efd19cdd62b2ff57
Assignee | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 14•11 years ago
|
||
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 → ---
Comment 15•11 years ago
|
||
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•11 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•11 years ago
|
||
Assignee | ||
Comment 18•11 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)
Comment 19•11 years ago
|
||
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 20•11 years ago
|
||
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)
Comment 21•11 years ago
|
||
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•11 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•11 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•11 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 25•11 years ago
|
||
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+
Updated•11 years ago
|
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•11 years ago
|
||
Hopefully last hotfix to fix all issues with the localId=0 hack removal.
Attachment #8335621 -
Flags: review?(kgrandon)
Comment 27•11 years ago
|
||
Comment on attachment 8335621 [details] [review]
Pull request 13889
Landed in master: https://github.com/mozilla-b2g/gaia/commit/298e6c5af4fe0cc0c4cfd88f7973bdd08e33aa51
Attachment #8335621 -
Flags: review?(kgrandon) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•