Engineering mode framework

RESOLVED FIXED in Firefox 35

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jj.evelyn, Assigned: jj.evelyn)

Tracking

({dev-doc-needed})

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 wontfix, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [Tako_Blocker])

Attachments

(7 attachments, 16 obsolete attachments)

46 bytes, text/x-github-pull-request
rik
: review+
Details | Review
7.94 KB, patch
Details | Diff | Splinter Review
17.17 KB, patch
sicking
: superreview+
Details | Diff | Splinter Review
1.20 KB, patch
jj.evelyn
: review+
Details | Diff | Splinter Review
3.45 KB, patch
jj.evelyn
: review+
Details | Diff | Splinter Review
17.25 KB, patch
Details | Diff | Splinter Review
8.79 KB, patch
Details | Diff | Splinter Review
Assignee

Description

5 years ago
we need a solution to launch engineering mode app.
Assignee

Comment 1

5 years ago
generally speaking, vendor will dial a special code from dialer app to trigger their engineering mode app. Instead of checking the code in dialer app and using web activity to launch their eng-mode app, I suggest moving the whole checking rule to gecko. It may be safer - compare to digging the code out from dialer's app zip, it's harder to find it in Gecko files wrapped in onmi.ja  (I know of course if anyone wants to, he/she always can find it there. I'm claiming it's "harder", not impossible. Correct me if I think it wrong)

So here is my suggestion:

in /gecko/dom/telephony/gonk/TelephonyProvider.js

419   dial: function(aClientId, aNumber, aIsEmergency) {
+       const engmodeMagicNumber = "1234****";
+       if (aNumber === engmodeMagicNumber) {
+         var disconnectedCall = {"state":0,"callIndex":1,"toa":129,"isMpty":false,
+           "isMT":false,"als":0,"isVoice":true,"isVoicePrivacy":false,
+           "number":engmodeMagicNumber,"numberPresentation":0,"name":"",
+           "namePresentation":0,"uusInfo":null,"direction":"outgoing","started":0,
+           "rilRequestType":18,"rilRequestError":0}
+
+         this.handleCallDisconnected(disconnectedCall);
+         return;
+       }
420     if (DEBUG) debug("Dialing " + (aIsEmergency ? "emergency " : "") + aNumber);

and we need fire a system message to trigger eng-mode app in the if condition. I'm figuring out the how-to.
Add RIL and engineer mode owner here.
blocking-b2g: --- → 1.3T?
Assignee

Comment 3

5 years ago
(In reply to Evelyn Hung [:evelyn] from comment #1)
> generally speaking, vendor will dial a special code from dialer app to
> trigger their engineering mode app. Instead of checking the code in dialer
> app and using web activity to launch their eng-mode app, I suggest moving
> the whole checking rule to gecko. It may be safer - compare to digging the
> code out from dialer's app zip, it's harder to find it in Gecko files
> wrapped in onmi.ja  (I know of course if anyone wants to, he/she always can
> find it there. I'm claiming it's "harder", not impossible. Correct me if I
> think it wrong)
> 

So I was told that either in Gaia or in Gecko is in the same security level - it's not secure at all. Okay, agree. I won't insist on my suggestion above. I hope it's a WIP patch on SPRD side only.
We think put code of entry engineer mode to gaia is better, because we have no permission to merge code to gecko. Thanks.
Gene would work on an example of communication between engineer mode app and chrome code (shell.js or XPCOM component) with IAC.  Gene would provide

 1. An example app connect to the IAC message port of keyword "engine-mode".
 2. An XPCOM component in JS provide the service for the keyword "engine-mode" through IAC.

1) would send a "PING" message to the message port, then 2) reply a "PONG" message for "PING".
This is assigned to gene until he finish his part.
Assignee: nobody → gene.lian
After discussion with Gene and Thinker, we are going this way(Please correct me if I get something wrong):
1. Dialer app invoke moz activity to start engineering mode app
2. the XPCOM component will get notified by moz activity in step 1, then it will establish an IAC connection with engineering mode app

I'll start working on step 1.
triage: this feature will not land in Mozilla repo. minus
blocking-b2g: 1.3T? → -
Posted patch Patch (WIP) (obsolete) — Splinter Review
Hi Thinker, this is a very quick WIP (not yet tested). Would you please take a look to ensure if I'm in the right direction or not? Thanks!
Attachment #8410747 - Flags: feedback?(tlee)
As the discussion offline, Tzu-Lin would implement an engineer mode app triggered by the dialer through activity.  The xpcom component implemented by gene would wait for the system message of the activity and connect to the app with an IAC connection.
Attachment #8410747 - Flags: feedback?(tlee)
Assignee

Comment 11

5 years ago
Warning: The patch here is just for partner reference to know how to expose hardware information to application. The patch hasn't been tested nor reviewed to ensure it's secure.
Assignee: gene.lian → nobody
Assignee: nobody → echou
This new patch is based on Gene's patch and do the following things:

(1) Create and initialize Eng mode handler in Gecko. Just like what Gene did.
(2) Listen to MozActivity in ActivitiesService.jsm. Once the activity with name "engineering-mode" is fired, do engineeringModeHandler.connect() to actively establish a connection with Engineering Mode app.
Posted patch Dialer-306.patch (obsolete) — Splinter Review
Gaia patch provided by Evelyn. It fires a MozActivity with name "engineering-mode" after dialing "306" from Dialer.
Temp EngineeringMode app which provides super basic test of the IAC connection.
Assignee

Comment 15

5 years ago
The following requirement is an offline discussion between a few people including partners, and written down by Jonas. It described what we are going to do in this bug.

====

A) Add a gaia-setting for "key combination in dialer that triggers
service mode".

B) Change the dialer app to read this setting and wait for the correct
key combination to be triggered.

C) When the combination has been hit, let the dialer app launch an
activity called "internal-system-engineering-mode" or some such.

D) Change web activities to enforce that activity names starting with
"internal-system-" can only be initiated and handled by certified
apps.

E) Add a XPCOM interface for engineering mode. The API would have
functions for getValue/setValue/message.

F) Enable some way for a certified app to get a connection to above
XPCOM interface. This includes doing the IPC between child/parent
process as the XPCOM interface will run in the parent, but the app
runs in the child, do necessary security checks in the parent, forward
calls to XPCOM interface.
Assignee

Comment 16

5 years ago
renamed bug title since it's a general solution for all partners.
Summary: [tarako] launch engineering mode app → launch engineering mode app
Assignee

Updated

5 years ago
Attachment #8410747 - Attachment is obsolete: true
Assignee

Comment 17

5 years ago
The patch does A-C in comment 15.
I use a setting for the key combination and the key can be set by customization, which means by default the setting does not exist and no engineering mode app will be triggered.
Attachment #8464620 - Flags: review?(etienne)
Attachment #8464620 - Flags: feedback?(jonas)
Looks like the design of this feature is settled. Could you open blocking bugs for each part? For the dialer part, we need to make sure that we don't slow down opening the dialer nor tapping on the keypad nor calling.
Assignee

Comment 19

5 years ago
(In reply to Anthony Ricaud (:rik) from comment #18)
> Looks like the design of this feature is settled. Could you open blocking
> bugs for each part? For the dialer part, we need to make sure that we don't
> slow down opening the dialer nor tapping on the keypad nor calling.

We plan to land both gecko and gaia parts in this bug so the story is more complete and easier for partner to understand. The design looks complicate but the implementation is actually not that huge.
Assignee

Comment 20

5 years ago
(In reply to Evelyn Hung [:evelyn] from comment #19)
> (In reply to Anthony Ricaud (:rik) from comment #18)
> > Looks like the design of this feature is settled. Could you open blocking
> > bugs for each part? For the dialer part, we need to make sure that we don't
> > slow down opening the dialer nor tapping on the keypad nor calling.
> 
> We plan to land both gecko and gaia parts in this bug so the story is more
> complete and easier for partner to understand. The design looks complicate
> but the implementation is actually not that huge.

I've submitted the patch of dialer part, let me know how can I help on launch time profiling. Thanks!
Assignee

Updated

5 years ago
Attachment #8463921 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8463920 - Attachment is obsolete: true
Assignee

Comment 21

5 years ago
(In reply to Evelyn Hung [:evelyn] from comment #17)
> Created attachment 8464620 [details] [review]
> EngMode-Gaia-Implementation
> 
> The patch does A-C in comment 15.
> I use a setting for the key combination and the key can be set by
> customization, which means by default the setting does not exist and no
> engineering mode app will be triggered.

so for testing this patch, add GAIA_DISTRIBUTION_DIR=customization for a build
This patch is Gecko side implementation of engineering mode. An application will be initiated and an IAC connection will be actively established after receiving mozActivity called "internal-system-engineering-mode".
Attachment #8463918 - Attachment is obsolete: true
Attachment #8466006 - Flags: feedback?(jonas)
Eli: Can you help us with launch time profiling of attachment 8464620 [details] [review]?
Comment on attachment 8464620 [details] [review]
EngMode-Gaia-Implementation

Stealing the review from Etienne as he is on PTO.

This is good but we'll need new tests to verify this.
Attachment #8464620 - Flags: review?(etienne) → feedback+
I posted a proposal to dev-b2g for an approach that I think could work. It uses WebActivities rather than IAC to launch the engineering mode app, but I think IAC could work too.

Though the advantage of using WebActivities is that we've needed a solution for "system only webactivities" many times in the past, so I think it could save us time elsewhere if we had it.

https://groups.google.com/forum/#!topic/mozilla.dev.b2g/CimpYNu7soQ
Assignee

Comment 26

5 years ago
Open this bug to public since we already discussed on dev-b2g. :)
Group: mozilla-employee-confidential
Assignee

Comment 27

5 years ago
(In reply to Anthony Ricaud (:rik) from comment #24)
> Comment on attachment 8464620 [details] [review]
> EngMode-Gaia-Implementation
> 
> Stealing the review from Etienne as he is on PTO.
> 
> This is good but we'll need new tests to verify this.

Thanks for the review. I will add tests in my next patch. :)
Of course I forgot to needinfo Eli for comment 23.
Flags: needinfo?(eperelman)

Comment 29

5 years ago
Hey Rik, I'd be happy to. I'll run some tests against the Dialer app and report back the results here.
Assignee

Comment 30

5 years ago
Take this bug. I will try to finish both Gecko and Gaia parts.
Assignee: echou → ehung
Assignee

Comment 31

5 years ago
Comment on attachment 8466006 [details] [diff] [review]
patch 1: v1: engineering mode gecko support

cancel feedback? since Jonas had left some comments.
Attachment #8466006 - Flags: feedback?(jonas)
Assignee

Comment 32

5 years ago
Comment on attachment 8464620 [details] [review]
EngMode-Gaia-Implementation

same here.
Attachment #8464620 - Flags: feedback?(jonas)
Assignee

Comment 33

5 years ago
(In reply to :Eli Perelman from comment #29)
> Hey Rik, I'd be happy to. I'll run some tests against the Dialer app and
> report back the results here.

Hi Eli, 
  to enable my patch, you need to set |GAIA_DISTRIBUTION_DIR=customization| for build. Thank you for the profiling. :)

Comment 34

5 years ago
Using the command: APP=communications/dialer RUNS=10 make test-perf

Against master, no patches:

"mozPerfDurationsAverage": 738.9307654999999

Against master with Evelyn's patch and Eric's patch:

"mozPerfDurationsAverage": 736.4642812

The launch time for the dialer app is negligible in my testing. My assessment is these patches have no ill affect on launch times.
Status: NEW → ASSIGNED
Flags: needinfo?(eperelman)
Comment on attachment 8466006 [details] [diff] [review]
patch 1: v1: engineering mode gecko support

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

I replied to the thread on b2g on this, but Ill comment here as well just for reference:

So to summarise the security requirements &properties as proposed by Jonas on the b2g thread:

- Engineering mode will be disabled until the dialer (or other certified app) initiates the special web activity
- Even with engineering mode enabled, only an app with the certified “engineering-mode” (or similar) permission can access the API which can be extended by the vendor

I’d make a couple notes here:
- There is no reason for side loaded apps to have this permission, and its present a very real threat of local privilege escalation,  so we should explicitly prevent side loading apps which have access to these APIs (perhaps so long as the devtools.debugger.forbid-certified-apps pref is true, so engineering build phones can still access them). 
- The dialer should take observe lock state - we need to prevent enabling engineering mode from the dialer presented by the lockscreen ( i think your patch does this already though since it only changes the dialer, not the lockscreen dialer)

We should also provide security guidance to Partners implementing engineering mode functionality to reduce the risk of security issues here. Securing the API is one thing, but there is still the possibility of the Engineering App exposing other inputs which leads to compromise. For example, an issue seen in the past was apps read files from writable locations and then inserting the contents into HTML (XSS).

::: dom/activities/src/ActivitiesService.jsm
@@ +210,5 @@
> +    // Special handling for launching Engineering Mode app from shell.js
> +    if (aMsg.options.name == "internal-system-engineering-mode") {
> +      Services.obs.notifyObservers(null, "internal-system-engineering-mode", null);
> +      return;
> +    }

Evelyn, are you going to add some security checks here to ensure that this activity was initiated by a certified app? I assume so based on the email discussion on b2g. "[b2g] Engineering Mode (aka "service menu")" Otherwise any web page can initiate engineering mode.

::: dom/apps/src/EngineeringModeHandler.js
@@ +42,5 @@
> +   * the string 'test'. Once related operations have been done and you
> +   * want to reply to Gaia EM app, just use sendMessage() to send data back
> +   * through the tunnel.
> +   */
> +};

I assume we are going to replace IAC with a proper API here. This is important as otherwise if an attacker compromises any certified app, they can they connect to engineering mode which might provide a path for privileged escalation (e.g executing arbitrary system commands instead of just arbitrary JavaScript)
Assignee

Comment 36

5 years ago
Hi Paul,
Thanks for giving advice, I always appreciate your input on security part.

(In reply to Paul Theriault [:pauljt] from comment #35)
> Comment on attachment 8466006 [details] [diff] [review]
> patch 1: v1: engineering mode gecko support
> 
> Review of attachment 8466006 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I replied to the thread on b2g on this, but Ill comment here as well just
> for reference:
> 
> So to summarise the security requirements &properties as proposed by Jonas
> on the b2g thread:
> 
> - Engineering mode will be disabled until the dialer (or other certified
> app) initiates the special web activity
> - Even with engineering mode enabled, only an app with the certified
> “engineering-mode” (or similar) permission can access the API which can be
> extended by the vendor
> 
> I’d make a couple notes here:
> - There is no reason for side loaded apps to have this permission, and its
> present a very real threat of local privilege escalation,  so we should
> explicitly prevent side loading apps which have access to these APIs
> (perhaps so long as the devtools.debugger.forbid-certified-apps pref is
> true, so engineering build phones can still access them). 

Noted. Thanks.

Per our app sandbox design, as long as the engineering mode app doesn't get embed-apps and webapp-manager permission, the side loaded app won't be able to run in the same process nor have the same permission.
I don't see the case that a engineering mode app needs these two permissions, but maybe a review process on (at least) the app's manifest to prevent it happens.

> - The dialer should take observe lock state - we need to prevent enabling
> engineering mode from the dialer presented by the lockscreen ( i think your
> patch does this already though since it only changes the dialer, not the
> lockscreen dialer)
> 

Yes, I checked, the Emergency call app uses its own logic to make a emergency call.

> We should also provide security guidance to Partners implementing
> engineering mode functionality to reduce the risk of security issues here.
> Securing the API is one thing, but there is still the possibility of the
> Engineering App exposing other inputs which leads to compromise. For
> example, an issue seen in the past was apps read files from writable
> locations and then inserting the contents into HTML (XSS).
> 
> ::: dom/activities/src/ActivitiesService.jsm
> @@ +210,5 @@
> > +    // Special handling for launching Engineering Mode app from shell.js
> > +    if (aMsg.options.name == "internal-system-engineering-mode") {
> > +      Services.obs.notifyObservers(null, "internal-system-engineering-mode", null);
> > +      return;
> > +    }
> 
> Evelyn, are you going to add some security checks here to ensure that this
> activity was initiated by a certified app? I assume so based on the email
> discussion on b2g. "[b2g] Engineering Mode (aka "service menu")" Otherwise
> any web page can initiate engineering mode.
> 

Yes, I will add a security check to meet our requirement.

> ::: dom/apps/src/EngineeringModeHandler.js
> @@ +42,5 @@
> > +   * the string 'test'. Once related operations have been done and you
> > +   * want to reply to Gaia EM app, just use sendMessage() to send data back
> > +   * through the tunnel.
> > +   */
> > +};
> 
> I assume we are going to replace IAC with a proper API here. This is
> important as otherwise if an attacker compromises any certified app, they
> can they connect to engineering mode which might provide a path for
> privileged escalation (e.g executing arbitrary system commands instead of
> just arbitrary JavaScript)

I don't get your idea, I thought in our imagination of the API, it also allows arbitrary system commands via getValue/setValue functions? 

The XPCOM interface would look something like:

// Implemented by engineering-mode library
interface nsIEngineeringMode
{
  DOMString getValue(in DOMString name);
  void setValue(in DOMString name, in DOMString value);
  void setMessageHandler(in nsIEngineeringModeMessageHandler handler);
}

interface nsIEngineeringModeMessageHandler
{
  void handleMessage(DOMString message);
} 

I didn't see the difference between this API and IAC.
(In reply to Evelyn Hung [:evelyn] from comment #36)
> Hi Paul,
> Thanks for giving advice, I always appreciate your input on security part.
> 
> (In reply to Paul Theriault [:pauljt] from comment #35)
> > Comment on attachment 8466006 [details] [diff] [review]
> > patch 1: v1: engineering mode gecko support
>  
> ...
> > 
> > I’d make a couple notes here:
> > - There is no reason for side loaded apps to have this permission, and its
> > present a very real threat of local privilege escalation,  so we should
> > explicitly prevent side loading apps which have access to these APIs
> > (perhaps so long as the devtools.debugger.forbid-certified-apps pref is
> > true, so engineering build phones can still access them). 
> 
> Noted. Thanks.
> 
> Per our app sandbox design, as long as the engineering mode app doesn't get
> embed-apps and webapp-manager permission, the side loaded app won't be able
> to run in the same process nor have the same permission.
> I don't see the case that a engineering mode app needs these two
> permissions, but maybe a review process on (at least) the app's manifest to
> prevent it happens.

Maybe I misunderstood - are we going to have a special "engineering-mode" permission to access nsIEngineeringMode API? I assumed so, but maybe I got it wrong?

What I meant was that we should change to the devtools actor that is responsible for installing apps during sideloading to forbid sideloading an app with this permission. I.E this actor should prevent a sideloadeding apps from being granted this permission, if devtools.debugger.forbid-certified-apps is set to true. 


> 
> > - The dialer should take observe lock state - we need to prevent enabling
> > engineering mode from the dialer presented by the lockscreen ( i think your
> > patch does this already though since it only changes the dialer, not the
> > lockscreen dialer)
> > 
> 
> Yes, I checked, the Emergency call app uses its own logic to make a
> emergency call.

Thanks for checking this.

> 
> > We should also provide security guidance to Partners implementing
> > engineering mode functionality to reduce the risk of security issues here.
> > Securing the API is one thing, but there is still the possibility of the
> > Engineering App exposing other inputs which leads to compromise. For
> > example, an issue seen in the past was apps read files from writable
> > locations and then inserting the contents into HTML (XSS).
> > 
> > ::: dom/activities/src/ActivitiesService.jsm
> > @@ +210,5 @@
> > > +    // Special handling for launching Engineering Mode app from shell.js
> > > +    if (aMsg.options.name == "internal-system-engineering-mode") {
> > > +      Services.obs.notifyObservers(null, "internal-system-engineering-mode", null);
> > > +      return;
> > > +    }
> > 
> > Evelyn, are you going to add some security checks here to ensure that this
> > activity was initiated by a certified app? I assume so based on the email
> > discussion on b2g. "[b2g] Engineering Mode (aka "service menu")" Otherwise
> > any web page can initiate engineering mode.
> > 
> 
> Yes, I will add a security check to meet our requirement.
> 
> > ::: dom/apps/src/EngineeringModeHandler.js
> > @@ +42,5 @@
> > > +   * the string 'test'. Once related operations have been done and you
> > > +   * want to reply to Gaia EM app, just use sendMessage() to send data back
> > > +   * through the tunnel.
> > > +   */
> > > +};
> > 
> > I assume we are going to replace IAC with a proper API here. This is
> > important as otherwise if an attacker compromises any certified app, they
> > can they connect to engineering mode which might provide a path for
> > privileged escalation (e.g executing arbitrary system commands instead of
> > just arbitrary JavaScript)
> 
> I don't get your idea, I thought in our imagination of the API, it also
> allows arbitrary system commands via getValue/setValue functions? 

Again maybe I reviewed this code wrong. It looked like it was using IAC to communicate, and I was just making sure that we move to an XPCOM API as per jonas' email. Sounds like you were already on the right track.

> 
> The XPCOM interface would look something like:
> 
> // Implemented by engineering-mode library
> interface nsIEngineeringMode
> {
>   DOMString getValue(in DOMString name);
>   void setValue(in DOMString name, in DOMString value);
>   void setMessageHandler(in nsIEngineeringModeMessageHandler handler);
> }
> 
> interface nsIEngineeringModeMessageHandler
> {
>   void handleMessage(DOMString message);
> } 
> 
> I didn't see the difference between this API and IAC.

The difference is that an API can be restricted by permissions. IAC can technically be restricted by rules, but there is some debate about these (bug 1019493). So I would prefer an XPCOM API, with an standard permission check in the parent, so that we aren't relying on IAC rules to protect this feature.

For example, if you use IAC instead of permissions, you might restrict access to engineering.gaiamobile.org. An attacker with access to the phone could sideload an app with this origin (by setting it in the manifest) and thus gain access to the engineering mode APIs.
Assignee

Comment 38

5 years ago
(In reply to Paul Theriault [:pauljt] from comment #37)
> (In reply to Evelyn Hung [:evelyn] from comment #36)
> 
> Maybe I misunderstood - are we going to have a special "engineering-mode"
> permission to access nsIEngineeringMode API? I assumed so, but maybe I got
> it wrong?
>

I wasn't planing to, but yes you're right. An EngineeringMode API should have a corresponding "engineering-mode" permission which is for certified app only.
 
> What I meant was that we should change to the devtools actor that is
> responsible for installing apps during sideloading to forbid sideloading an
> app with this permission. I.E this actor should prevent a sideloadeding apps
> from being granted this permission, if
> devtools.debugger.forbid-certified-apps is set to true. 
> 

Yes. agree. (sorry I got your idea wrong, so please ignore my previous comment.;) 
As I know, we can't install a certified app via our devtool, can we? So I think it's as safe as other certified API permissions.

> 
> The difference is that an API can be restricted by permissions. IAC can
> technically be restricted by rules, but there is some debate about these
> (bug 1019493). So I would prefer an XPCOM API, with an standard permission
> check in the parent, so that we aren't relying on IAC rules to protect this
> feature.
> 
> For example, if you use IAC instead of permissions, you might restrict
> access to engineering.gaiamobile.org. An attacker with access to the phone
> could sideload an app with this origin (by setting it in the manifest) and
> thus gain access to the engineering mode APIs.

Okay, I'm convinced. The effort of reusing IAC to establish a connection "from chrome to a certified app" is not less than implementing a new API, and this mechanism (chrome to a certified app IAC connection) is not easy to be extended to other use cases because it depends on a trigger point so chrome knows when and who to connect...

I'm implementing the API and the whole engineering mode framework will exactly follow Jonas' and your idea. Thanks for discussing and explaining these to me. I will set flags to you both when my patch is done.
Assignee

Comment 39

5 years ago
Comment on attachment 8466006 [details] [diff] [review]
patch 1: v1: engineering mode gecko support

This patch is not a suggested way now.
Attachment #8466006 - Attachment is obsolete: true
Assignee

Comment 40

5 years ago
The patch implements Engineering Mode WebAPI, includes a set of DOM API for content process, and a service component in b2g process to handle requests from content process and then delegate to partner's engineering mode component.

Things aren't included in this patch:
1. test case. I'm not sure how to test it, would like to hear your idea.
2. System only web activity (will be a separated patch)
3. A component implementing nsIEngineeringMode.idl for partners' reference (will be a separated patch)
Attachment #8478177 - Flags: feedback?(ptheriault)
Attachment #8478177 - Flags: feedback?(jonas)
Assignee

Comment 41

5 years ago
This patch covers comment 40 point #3:
> 3. A component implementing nsIEngineeringMode.idl for partners' reference
Attachment #8478180 - Flags: feedback?(ptheriault)
Attachment #8478180 - Flags: feedback?(jonas)
(In reply to Evelyn Hung [:evelyn] from comment #38)
> (In reply to Paul Theriault [:pauljt] from comment #37)
> > (In reply to Evelyn Hung [:evelyn] from comment #36)
> > 
> > Maybe I misunderstood - are we going to have a special "engineering-mode"
> > permission to access nsIEngineeringMode API? I assumed so, but maybe I got
> > it wrong?
> >
> 
> I wasn't planing to, but yes you're right. An EngineeringMode API should
> have a corresponding "engineering-mode" permission which is for certified
> app only.
>  
> > What I meant was that we should change to the devtools actor that is
> > responsible for installing apps during sideloading to forbid sideloading an
> > app with this permission. I.E this actor should prevent a sideloadeding apps
> > from being granted this permission, if
> > devtools.debugger.forbid-certified-apps is set to true. 
> > 
> 
> Yes. agree. (sorry I got your idea wrong, so please ignore my previous
> comment.;) 
> As I know, we can't install a certified app via our devtool, can we? So I
> think it's as safe as other certified API permissions.

That's not the case - certified apps can be side loaded via the devtools actor since version 1.2. 

Paul/Fabrice, since engineering mode can be _anything_ that a partner implements it is very dangerous. My thought was that the devtools actor could disallow installing an app with this permission unless the devtools.debugger.forbid-certified-apps is set to false. Basically I want the only way to get engineering-mode permission is by flashing gaia - is this the best way to achieve this ?
Flags: needinfo?(paul)
Flags: needinfo?(fabrice)
(In reply to Evelyn Hung [:evelyn] from comment #40)
> Created attachment 8478177 [details] [diff] [review]
> Gecko-implementation-of-Engineering-mode-WebAPI-v1.patch
> 
> The patch implements Engineering Mode WebAPI, includes a set of DOM API for
> content process, and a service component in b2g process to handle requests
> from content process and then delegate to partner's engineering mode
> component.
> 
> Things aren't included in this patch:
> 1. test case. I'm not sure how to test it, would like to hear your idea.
> 2. System only web activity (will be a separated patch)
> 3. A component implementing nsIEngineeringMode.idl for partners' reference
> (will be a separated patch)

Looks good to me so far. I assume the system-only web activity will somehow enable/disable engineering-mode API- is that the plan?
Comment on attachment 8478177 [details] [diff] [review]
Gecko-implementation-of-Engineering-mode-WebAPI-v1.patch

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

The general approach seems good. Though someone that has more experience in JS implemented APIs should review.

And we need to address Paul's security concerns, but that's probably a different patch.

::: dom/engineeringmode/EngineeringModeService.js
@@ +66,5 @@
> +    }
> +  },
> +
> +  receiveMessage: function(aMessage) {
> +    debug("receiveMessage: name: " + aMessage.name);

Don't you need some security checks here? To make sure that the child process has access to use this API? It's important that we do those checks before the Engineering mode XPCOM component gets instantiated.
Attachment #8478177 - Flags: feedback?(jonas) → feedback+
(In reply to Paul Theriault [:pauljt] from comment #42)

> Paul/Fabrice, since engineering mode can be _anything_ that a partner
> implements it is very dangerous. My thought was that the devtools actor
> could disallow installing an app with this permission unless the
> devtools.debugger.forbid-certified-apps is set to false. Basically I want
> the only way to get engineering-mode permission is by flashing gaia - is
> this the best way to achieve this ?

We can totally forbid installing apps with this permission from the devtools actor if needed. That's the only way to force preloading as part of gaia. Is that what you want?
Flags: needinfo?(fabrice)
Comment on attachment 8478177 [details] [diff] [review]
Gecko-implementation-of-Engineering-mode-WebAPI-v1.patch

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

::: dom/engineeringmode/EngineeringModeAPI.js
@@ +39,5 @@
> +    this._window = aWindow;
> +    let util = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +               .getInterface(Ci.nsIDOMWindowUtils);
> +    this._windowId = util.currentInnerWindowID;
> +    Services.obs.addObserver(this, "inner-window-destroyed", false);

DOMRequestHelper does that for you already.

@@ +42,5 @@
> +    this._windowId = util.currentInnerWindowID;
> +    Services.obs.addObserver(this, "inner-window-destroyed", false);
> +
> +    cpmm.sendAsyncMessage("EngineeringMode:Register", null);
> +    cpmm.addMessageListener("EngineeringMode:ComingMessage", this);

Any reason to not register this message in initDOMRequestHelper?
Nit: I would use EngineeringMode:OnMessage, but naming is hard!

@@ +50,5 @@
> +  _cleanup: function() {
> +    cpmm.removeMessageListener("EngineeringMode:ComingMessage");
> +    cpmm.sendAsyncMessage("EngineeringMode:Unregister", null);
> +    Services.obs.removeObserver(this, "inner-window-destroyed");
> +  },

rename this function to uninit(), and it will be called by DOMRequestHelper when needeed. You don't even need to remove the message listeners for you.

@@ +100,5 @@
> +    debug("receiveMessage: name: " + aMessage.name);
> +
> +    switch (aMessage.name) {
> +      case "EngineeringMode:ComingMessage":
> +        let detail = Cu.cloneInto(aMessage.json, this._window);

nit: use aMessage.data

@@ +111,5 @@
> +        if (!resolver) {
> +          return;
> +        }
> +        resolver.resolve(aMessage.json.value);
> +        break;

Could there be cases where we would reject the promise?

::: dom/engineeringmode/EngineeringModeService.js
@@ +67,5 @@
> +  },
> +
> +  receiveMessage: function(aMessage) {
> +    debug("receiveMessage: name: " + aMessage.name);
> +

Yep, we need to do a |aMessage.target.assertPermission("engineering-mode")| here.

But that will still be after we instantiate the xpcom component (that happens in _hasEngineeringModeImpl(), called when we get the profile-after-change observer notification). But see below.

@@ +74,5 @@
> +    }
> +
> +    switch (aMessage.name) {
> +      case "EngineeringMode:Register":
> +        this._clients.push(aMessage.target);

you could instantiate the xpcom only when you first register a client, ie. moving EngineeringMode.setMessageHandler(...) here.

@@ +101,5 @@
> +    }
> +  },
> +
> +  _hasEngineeringModeImpl: function() {
> +    if (typeof EngineeringMode === "undefined") {

To not instantiate the component, test if Cc["@mozilla.org/b2g/engineering-mode-impl;1"] is defined.
(In reply to Jonas Sicking (:sicking) from comment #44)
> Comment on attachment 8478177 [details] [diff] [review]
> Gecko-implementation-of-Engineering-mode-WebAPI-v1.patch
> 
> Review of attachment 8478177 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The general approach seems good. Though someone that has more experience in
> JS implemented APIs should review.
> 
> And we need to address Paul's security concerns, but that's probably a
> different patch.
> 
> ::: dom/engineeringmode/EngineeringModeService.js
> @@ +66,5 @@
> > +    }
> > +  },
> > +
> > +  receiveMessage: function(aMessage) {
> > +    debug("receiveMessage: name: " + aMessage.name);
> 
> Don't you need some security checks here? To make sure that the child
> process has access to use this API? It's important that we do those checks
> before the Engineering mode XPCOM component gets instantiated.

I was going to suggest this, but then I noticed the type and permission restriction in the webIDL file. Ie

NavigatorProperty="mozEngineeringMode",
AvailableIn=CertifiedApps,
CheckPermissions="engineering-mode"]

Not sure what this does though - from [1] it just looks like a check in the child process? That's not ideal - I wonder if its possible to have the binding utils enforce a check in the parent process as well. 

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.cpp#2360


http://dxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.cpp#2360
Flags: needinfo?(paul)
So to be clear Jonas answered my question offline: the CheckPermissions is only a permissions check in the child process. So definitely need to add a line like aMessage.target.hasPermission('engineering-mode') here in the parent.
Assignee

Comment 49

5 years ago
(In reply to Fabrice Desré [:fabrice] from comment #46)
> Comment on attachment 8478177 [details] [diff] [review]
> Gecko-implementation-of-Engineering-mode-WebAPI-v1.patch
> 
> Review of attachment 8478177 [details] [diff] [review]:

Hey Fabrice! I was thinking to ask your review after I polish it (remove those redundant code that DOMRequestHelper already did). Thanks for jumping in and leaving feedback.


(In reply to Paul Theriault [:pauljt] from comment #48)
> So to be clear Jonas answered my question offline: the CheckPermissions is
> only a permissions check in the child process. So definitely need to add a
> line like aMessage.target.hasPermission('engineering-mode') here in the
> parent.

Yes, Jonas's concern is right, I will do this check. Thank you both.
Assignee

Comment 50

5 years ago
(In reply to Paul Theriault [:pauljt] from comment #43)
> (In reply to Evelyn Hung [:evelyn] from comment #40)
> > Created attachment 8478177 [details] [diff] [review]
> > Gecko-implementation-of-Engineering-mode-WebAPI-v1.patch
> > 
> > The patch implements Engineering Mode WebAPI, includes a set of DOM API for
> > content process, and a service component in b2g process to handle requests
> > from content process and then delegate to partner's engineering mode
> > component.
> > 
> > Things aren't included in this patch:
> > 1. test case. I'm not sure how to test it, would like to hear your idea.
> > 2. System only web activity (will be a separated patch)
> > 3. A component implementing nsIEngineeringMode.idl for partners' reference
> > (will be a separated patch)
> 
> Looks good to me so far. I assume the system-only web activity will somehow
> enable/disable engineering-mode API- is that the plan?

We could do that, but it's a bit weird to me though. If system-only web activity enabled engineering-mode API, then when we disable it? :/
I thought certified-app-only check and preventing side-loaded certified app are enough. May I know your concern on enabling this API at run-time?
Flags: needinfo?(ptheriault)
> We could do that, but it's a bit weird to me though. If system-only web
> activity enabled engineering-mode API, then when we disable it? :/
> I thought certified-app-only check and preventing side-loaded certified app
> are enough. May I know your concern on enabling this API at run-time?

I had assumed it would be available until the device was restarted. This possibly isn't necessary though if we do the change which prevents side-loaded certified apps from accessing the engineering-mode API.

But I think I just misunderstood on what you are planning to do with the web activity handler - so can you please explain what will happen when the gecko (??)  handles "internal-system-engineering-mode" web activity? 

Let me rephrase my comments in terms of the threats I think we need to mitigate:

1. side-loading of an app which has access to engineering-mode API
This might allow local privilege escalation, so we should make sure that only apps that are included at build time/flashing gaia time can access this API (hence why I suggested the change to devtools, and I agree this is probably a separate bug)

2. vulnerabilities in vendor engineering mode applications leading to security issues.
If we disabled the engineering-mode API until the keycode was entered in the dialer, it wouldn't matter if the engineering mode apps have vulnerabilities, since the API itself wouldn't be enabled until the user types in the special code. 

Does that help? Maybe there are different ways to solve this threat, but let me know what you think.
Flags: needinfo?(ptheriault)
Assignee

Comment 52

5 years ago
Posted patch WIP-bug-997564-activity.patch (obsolete) — Splinter Review
> But I think I just misunderstood on what you are planning to do with the web > activity handler - so can you please explain what will happen when the gecko > (??)  handles "internal-system-engineering-mode" web activity? 

It's still a WIP and I haven't tested, but this is what I plan to do - just making sure both caller and callee are certified apps.
Assignee

Comment 53

5 years ago
(In reply to Paul Theriault [:pauljt] from comment #51) 
> 1. side-loading of an app which has access to engineering-mode API
> This might allow local privilege escalation, so we should make sure that
> only apps that are included at build time/flashing gaia time can access this
> API (hence why I suggested the change to devtools, and I agree this is
> probably a separate bug)
> 

Yep, it's out of this bug's scope, but I agree we should do that. Could you open a bug for this? (I'm afraid that I can't describe this issue as clear as you do)

> 2. vulnerabilities in vendor engineering mode applications leading to
> security issues.
> If we disabled the engineering-mode API until the keycode was entered in the
> dialer, it wouldn't matter if the engineering mode apps have
> vulnerabilities, since the API itself wouldn't be enabled until the user
> types in the special code. 
> 

So I think the decision we are going to make is: do we allow this engineering-mode API to be used by other certified app?(and of course it's not side-loaded) 
See if we bind this API with the keycode (precisely, the 'internal-system-engineering-mode' web activity), then other application has no way to use this API except registering itself as an 'internal-system-engineering-mode' activity handler. 

I vote for not adding this restriction because this API is a shortcut for partner to test all kinds of hardware functionality. They may have more than one app with different usage (and launch means). IMO, they should use this API and implement their app/component carefully, and it's their responsibility to make sure there is no vulnerabilities.
Assignee

Comment 54

5 years ago
Posted patch bug-997564-activity-v2.patch (obsolete) — Splinter Review
Tested and it works. I didn't find any test case for activities, but I will try to write one for checking our permission rules here.
Attachment #8480735 - Attachment is obsolete: true
Attachment #8481920 - Flags: feedback?(ptheriault)
Assignee

Comment 55

5 years ago
I found this bug when I was testing my patch in https://bugzilla.mozilla.org/attachment.cgi?id=8481920. |Services.DOMRequest.fireError| dispatches event synchronously but at this moment the DOMRequest haven't been returned.
Attachment #8481921 - Flags: review?(fabrice)
Assignee

Comment 56

5 years ago
addressed comment 44 and comment 46. Thanks for your feedback!
Attachment #8478177 - Attachment is obsolete: true
Attachment #8478177 - Flags: feedback?(ptheriault)
Attachment #8482380 - Flags: review?(ptheriault)
Attachment #8482380 - Flags: review?(fabrice)
Assignee

Comment 57

5 years ago
So I guess all I left are test cases, and more codes for partner's reference (which means enriching attachment 8478180 [details] [diff] [review])

Here is my plan:
1. test cases for EngineeringModeAPI.js and EngineeringModeService.js
2. test cases for 'internal-system-engineering-mode' activity to make sure only certified apps can be caller/callee. 
   => I haven't figured out how to install a certified app for mochitest, it looks like nobody did it before. :(
3. test case for dialer app to make sure it always works if the dialing keycode is set.
4. add one or two tests in ui-test app to actually use engineering mode API.
5. to achieve 4, I need to write more codes in reference implementation.

I can imagine there is a lot of work in (1), (4) and (5), so I'd like to open a follow-up bugs if possible.
(In reply to Evelyn Hung [:evelyn] from comment #57)

> 2. test cases for 'internal-system-engineering-mode' activity to make sure
> only certified apps can be caller/callee. 
>    => I haven't figured out how to install a certified app for mochitest, it
> looks like nobody did it before. :(

We have pre-installed certified apps in the default test profile (see http://mxr.mozilla.org/mozilla-central/source/testing/profiles/webapps_mochitest.json), but they have no real manifest...
Attachment #8481921 - Flags: review?(fabrice) → review+
Comment on attachment 8482380 [details] [diff] [review]
Gecko-implementation-of-Engineering-mode-WebAPI-v2.patch

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

Evelyn, I didn't look closely at the code as I'd like to be sure that we all agree on the API first.

::: dom/engineeringmode/nsIEngineeringMode.idl
@@ +13,5 @@
> +// Implemented by contract id @mozilla.org/b2g/engineering-mode-impl;1
> +[scriptable, uuid(1aa97d3f-a6da-4100-93af-72d9ac324146)]
> +interface nsIEngineeringMode : nsISupports
> +{
> +  void getValue(in DOMString name, in nsIEngineeringModeMessageHandler handler);

what should this return if the key property doesn't exist? It seems that returning a promise here would fulfill the needs better.

::: dom/webidl/EngineeringMode.webidl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +[JSImplementation="@mozilla.org/dom/engineering-mode-api;1",
> + NavigatorProperty="mozEngineeringMode",

I think we don't want to use the moz prefix anymore.

@@ +7,5 @@
> + AvailableIn=CertifiedApps,
> + CheckPermissions="engineering-mode"]
> +interface EngineeringMode : EventTarget {
> +  Promise<DOMString> getValue(DOMString name);
> +  void setValue(DOMString name, DOMString value);

Could there be cases where setting the value could fail and we want to be notified?
Also, if the content code runs:
engMode.setValue("key", "hello!");
getValue("key").then((value) => { console.log(value == "hello!") });

Does that work properly? The code EngineeringModeService.js doesn't seem to enforce that.
Attachment #8482380 - Flags: review?(fabrice)
Assignee

Comment 60

5 years ago
Thanks for your review, Fabrice.

As I know, the API is designed for providing a (correct and secure) way for partner so they can have means to access hardware (in most cases, get hardware status) which is not exposed by our WebAPI. The API works as a connection between engineering-mode app and engineering-mode module in Gecko, both sides are implemented by partner, with three basic ways to interact - set, get, and dispatch/listen to events. Of course we can design it properly and do as much error controls as we can think of, but I think errors won't and shouldn't happen in real case. Therefore, when I implemented this part, I decided the only thing I want to enforce is - don't block main process.

Also reply my other comments inline.


(In reply to Fabrice Desré [:fabrice] from comment #59)
> Comment on attachment 8482380 [details] [diff] [review]
> Gecko-implementation-of-Engineering-mode-WebAPI-v2.patch
> 
> Review of attachment 8482380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Evelyn, I didn't look closely at the code as I'd like to be sure that we all
> agree on the API first.
> 
> ::: dom/engineeringmode/nsIEngineeringMode.idl
> @@ +13,5 @@
> > +// Implemented by contract id @mozilla.org/b2g/engineering-mode-impl;1
> > +[scriptable, uuid(1aa97d3f-a6da-4100-93af-72d9ac324146)]
> > +interface nsIEngineeringMode : nsISupports
> > +{
> > +  void getValue(in DOMString name, in nsIEngineeringModeMessageHandler handler);
> 
> what should this return if the key property doesn't exist? It seems that
> returning a promise here would fulfill the needs better.
> 

Please not it's NOT a web facing idl.
In this case, the module implementer can return a pre-defined error code or null to indicate the key is invalid.

> ::: dom/webidl/EngineeringMode.webidl
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +[JSImplementation="@mozilla.org/dom/engineering-mode-api;1",
> > + NavigatorProperty="mozEngineeringMode",
> 
> I think we don't want to use the moz prefix anymore.
> 

Got it.

> @@ +7,5 @@
> > + AvailableIn=CertifiedApps,
> > + CheckPermissions="engineering-mode"]
> > +interface EngineeringMode : EventTarget {
> > +  Promise<DOMString> getValue(DOMString name);
> > +  void setValue(DOMString name, DOMString value);
> 
> Could there be cases where setting the value could fail and we want to be
> notified?

In this case, they can dispatch an event to indicate the operation doesn't succeed.

> Also, if the content code runs:
> engMode.setValue("key", "hello!");
> getValue("key").then((value) => { console.log(value == "hello!") });
> 
> Does that work properly? The code EngineeringModeService.js doesn't seem to
> enforce that. 

Yes, as I explained above, I didn't enforce this. In my opinion, though the API looks like a pair - get and set, but they actually don't have to be used in this relationship. So I leave it to a implementer decision.

Comment 61

5 years ago
We are having some trouble integrating your engineering mode patch into our code base released from Qualcomm (FFOS 2.0 ES1).

(1) Getting the javascript error "Cc[aContract] is undefined" while trying to get the "@mozilla.org/b2g/engineering-mode-impl;1" object in EngineeringModeService.js 
It looks like that this new XPCOM component is not registered.

How can I register this XPCOM component and make sure it be registerd on firefox os system.
                          
(2) Getting a compile error as below:
WebIDL.WebIDLError: error: invalid syntax, /home/administrator/ws09/linux/gecko/dom/webidl/EngineeringMode.webidl line 10:10 
Promise<DOMString> getValue(DOMString name); 

It look like our codebase about Promise definition is different your development code base.
Do you have a better way to help me solve this problem ?

Not sure if we missed anything, or need to apply some other patches since we are not working on the master.

Please advise.

This is for [Tako].
Assignee

Comment 62

5 years ago
(In reply to JohnsonLin from comment #61)
> We are having some trouble integrating your engineering mode patch into our
> code base released from Qualcomm (FFOS 2.0 ES1).
> 
> (1) Getting the javascript error "Cc[aContract] is undefined" while trying
> to get the "@mozilla.org/b2g/engineering-mode-impl;1" object in
> EngineeringModeService.js 
> It looks like that this new XPCOM component is not registered.
> 
> How can I register this XPCOM component and make sure it be registerd on
> firefox os system.

I didn't use "Cc[aContract]" in EngineeringModeService.js, could you show me the exact error message with line number?

BTW, you should also apply attachment 8478180 [details] [diff] [review] and start from modifying nsEngineeringMode.cpp.

>                           
> (2) Getting a compile error as below:
> WebIDL.WebIDLError: error: invalid syntax,
> /home/administrator/ws09/linux/gecko/dom/webidl/EngineeringMode.webidl line
> 10:10 
> Promise<DOMString> getValue(DOMString name); 
> 
> It look like our codebase about Promise definition is different your
> development code base.
> Do you have a better way to help me solve this problem ?

On v2.0, the syntax of webidl is slightly different. Could you remove '<DOMString>' and give it a shot?
Assignee

Comment 63

5 years ago
Hi Fabrice,
Please check my comment 60, and if my thought makes sense to you, would you like to review it again?
Flags: needinfo?(fabrice)

Comment 64

5 years ago
Hi Evelyn,
   We've applied attachment 8478180 [details] [diff] [review].

   The following log is from logcat:
    I/Gecko(204): -*- EngineeringModeService: -- init
    E/GeckoConsole(204): [JavaScript Error: "Cc[aContract] is undefined" {file: "resource://gre/modules/XPCOMUtils.jsm" line: 202}]
    
We also tried to test the element using ("@mozilla.org/b2g/engineering-mode-impl;1" in Components.classes)and it returns false.
  
Thanks.
(In reply to Evelyn Hung [:evelyn] from comment #60)
> Thanks for your review, Fabrice.
> 
> As I know, the API is designed for providing a (correct and secure) way for
> partner so they can have means to access hardware (in most cases, get
> hardware status) which is not exposed by our WebAPI. The API works as a
> connection between engineering-mode app and engineering-mode module in
> Gecko, both sides are implemented by partner, with three basic ways to
> interact - set, get, and dispatch/listen to events. Of course we can design
> it properly and do as much error controls as we can think of, but I think
> errors won't and shouldn't happen in real case. Therefore, when I
> implemented this part, I decided the only thing I want to enforce is - don't
> block main process.
> 
> Also reply my other comments inline.
> 
> 
> (In reply to Fabrice Desré [:fabrice] from comment #59)
> > Comment on attachment 8482380 [details] [diff] [review]
> > Gecko-implementation-of-Engineering-mode-WebAPI-v2.patch
> > 
> > Review of attachment 8482380 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Evelyn, I didn't look closely at the code as I'd like to be sure that we all
> > agree on the API first.
> > 
> > ::: dom/engineeringmode/nsIEngineeringMode.idl
> > @@ +13,5 @@
> > > +// Implemented by contract id @mozilla.org/b2g/engineering-mode-impl;1
> > > +[scriptable, uuid(1aa97d3f-a6da-4100-93af-72d9ac324146)]
> > > +interface nsIEngineeringMode : nsISupports
> > > +{
> > > +  void getValue(in DOMString name, in nsIEngineeringModeMessageHandler handler);
> > 
> > what should this return if the key property doesn't exist? It seems that
> > returning a promise here would fulfill the needs better.
> > 
> 
> Please not it's NOT a web facing idl.
> In this case, the module implementer can return a pre-defined error code or
> null to indicate the key is invalid.
> 
> > ::: dom/webidl/EngineeringMode.webidl
> > @@ +2,5 @@
> > > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > > +
> > > +[JSImplementation="@mozilla.org/dom/engineering-mode-api;1",
> > > + NavigatorProperty="mozEngineeringMode",
> > 
> > I think we don't want to use the moz prefix anymore.
> > 
> 
> Got it.
> 
> > @@ +7,5 @@
> > > + AvailableIn=CertifiedApps,
> > > + CheckPermissions="engineering-mode"]
> > > +interface EngineeringMode : EventTarget {
> > > +  Promise<DOMString> getValue(DOMString name);
> > > +  void setValue(DOMString name, DOMString value);
> > 
> > Could there be cases where setting the value could fail and we want to be
> > notified?
> 
> In this case, they can dispatch an event to indicate the operation doesn't
> succeed.
> 
> > Also, if the content code runs:
> > engMode.setValue("key", "hello!");
> > getValue("key").then((value) => { console.log(value == "hello!") });
> > 
> > Does that work properly? The code EngineeringModeService.js doesn't seem to
> > enforce that. 
> 
> Yes, as I explained above, I didn't enforce this. In my opinion, though the
> API looks like a pair - get and set, but they actually don't have to be used
> in this relationship. So I leave it to a implementer decision.

Evelyn, I'm not convinced by your arguments about the api design there. Let's get Ehsan to decide!
Flags: needinfo?(fabrice) → needinfo?(ehsan.akhgari)

Comment 66

5 years ago
On the issue of the web-facing setValue(), I also suggested on the dev-b2g thread that it should return a Promise<void> so that we can reject it if there is an error.  Jonas said that he's fine with it if there is a use case.  So if there is any reason why a setValue can fail (which I think there is, given that there is at least a whole bunch of IPC going on) this should return a Promise<void>.

On the issue of nsIEngineeringMode.getValue, I'm not sure if returning a promise from getValue itself makes sense (I'm not sure how well that plays with the XPConnect machinery), but if that works, that sounds fine.  But otherwise, getValue (and setValue for that matter, given the above) should accept some kind of a callback that the implementation will call to notify success/failure.  Then we will be able to hook up the implementation of the callback to resolve/reject the promise.

I hope this makes sense.  Please let me know if you have any questions!
Flags: needinfo?(ehsan.akhgari)
Assignee

Comment 67

5 years ago
Thanks for the feedback, Ehsan. So I summarized the points I got:

1. For web-facing API, both setValue() and getValue() should return a Promise.
2. For internal nsIEngineeringMode, both setValue() and getValue() should accept a callback for returning status of the operation.
Assignee

Comment 68

5 years ago
Hi Fabrice, per comment 65 and comment 67, this patch modified both web-facing API and internal API on setValue and getValue to ensure caller has better controls on callee's status.

> 1. For web-facing API, both setValue() and getValue() should return a Promise.
> 2. For internal nsIEngineeringMode, both setValue() and getValue() should accept a callback for returning status of the operation.
Attachment #8482380 - Attachment is obsolete: true
Attachment #8482380 - Flags: review?(ptheriault)
Attachment #8485856 - Flags: review?(fabrice)
Assignee

Comment 69

5 years ago
This patch is a default nsIEngineeringMode implementation for partner reference. I'm not sure how much I need to fill up, so now it just has comments and dummy data/errors returned.
BTW, I made it in C++ because partners are most likely to implement it in C++.
Attachment #8478180 - Attachment is obsolete: true
Attachment #8478180 - Flags: feedback?(ptheriault)
Attachment #8485872 - Flags: review?(fabrice)
Comment on attachment 8485856 [details] [diff] [review]
Gecko-implementation-of-Engineering-mode-WebAPI-v3.patch

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

That looks very good. Mostly nits, but I'd like to take another look before we land!

::: dom/engineeringmode/EngineeringModeAPI.js
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const DEBUG = true;

nit: set to false when landing.

@@ +30,5 @@
> +  classDescription: "Engineering Mode API",
> +  classID: Components.ID("{27e55b94-fc43-42b3-b0f0-28bebdd804f1}"),
> +  contractID: "@mozilla.org/dom/engineering-mode-api;1",
> +
> +  //For DOMRequestHelper: must have nsISupportsWeakReference and nsIObserver

nit: // For... and nsIObserver.

@@ +36,5 @@
> +                                         Ci.nsISupportsWeakReference,
> +                                         Ci.nsIObserver]),
> +
> +  init: function(aWindow) {
> +    this._window = aWindow;

DOMRequestIpcHelper already does that when you call initDOMRequestHelper

@@ +39,5 @@
> +  init: function(aWindow) {
> +    this._window = aWindow;
> +    let util = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +               .getInterface(Ci.nsIDOMWindowUtils);
> +    this._windowId = util.currentInnerWindowID;

Just use this.innerWindowID from the DOMRequestHelper

@@ +53,5 @@
> +  uninit: function() {
> +    cpmm.sendAsyncMessage("EngineeringMode:Unregister", null);
> +  },
> +
> +  // It returns a Promise

nit: // This returns a Promise<XXX>.

@@ +70,5 @@
> +
> +    return this.createPromise(promiseInit);
> +  },
> +
> +  // It returns a Promise

same here.

@@ +98,5 @@
> +  },
> +
> +  receiveMessage: function(aMessage) {
> +    debug("receiveMessage: name: " + aMessage.name);
> +    let resolver = null;

let data = aMessage.data and update the aMessage.data uses.

::: dom/engineeringmode/EngineeringModeService.js
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const DEBUG = true;

nit: please switch to false when landing.

@@ +67,5 @@
> +    }
> +
> +    switch (aMessage.name) {
> +      case "EngineeringMode:Register":
> +        // lazy bind message handler until we have first client

nit: Capital for the first comment letter, and end with a full stop.

@@ +92,5 @@
> +        break;
> +    }
> +  },
> +
> +  setValue: function(target, data) {

function(aTarget, aData)

@@ +116,5 @@
> +      }
> +    });
> +  },
> +
> +  getValue: function(target, data) {

function(aTarget, aData)

@@ +126,5 @@
> +      return;
> +    }
> +
> +    EngineeringMode.getValue(data.name, {
> +      onsuccess: function(value) {

function(aValue)

@@ +132,5 @@
> +          requestId: data.requestId,
> +          value: value
> +        });
> +      },
> +      onerror: function(error) {

function(aError)

::: dom/engineeringmode/EngineeringModeService.manifest
@@ +1,3 @@
> +component {1345a96a-7b8d-4017-a776-07d918f14d84} EngineeringModeService.js
> +contract @mozilla.org/engineering-mode-service;1 {1345a96a-7b8d-4017-a776-07d918f14d84}
> +category profile-after-change EngineeringModeService @mozilla.org/engineering-mode-service;1

I wouldn't mind merging this one in the other manifest. We need both components for anything to work anyway.

::: dom/engineeringmode/moz.build
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if CONFIG['MOZ_B2G']:

You already guard building the dom/engineeringmode in dom/moz.build so this CONFIG['MOZ_B2G'] is redundant. It doesn't hurt either, so feel free to keep it.
Another detail is whether we should use MOZ_B2G or MOZ_WIDGET_GONK... does that api make sense on b2g desktop? I honestly don't know!

@@ +17,5 @@
> +    ]
> +
> +    XPIDL_MODULE = 'dom_engineeringmode'
> +
> +# MOCHITEST_MANIFESTS += ['tests/mochitest.ini']

nit: remove that.
Attachment #8485856 - Flags: review?(fabrice) → feedback+
Comment on attachment 8485872 [details] [diff] [review]
[Do Not Checkin] Refererence-implementation-of-nsIEngineeringMode-v2.patch

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

Evelyn, thanks for doing that, that will be super useful for partners. We can't land that in our tree though, so there is no need for a formal review.
One thing that would be even better would be to explain how to build a standalone xpcom component, out of our tree since this is what we expect partners to ship.

::: dom/engineeringmode/nsEngineeringMode.cpp
@@ +27,5 @@
> +{
> +  if (!aCallback) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +  /* Write your corresponding implementation here, 

nit: trailing whitespace

@@ +59,5 @@
> +   *
> +   * e.g. aCallback->Onsuccess();
> +   * e.g. aCallback->Onerror(-1);
> +   *
> +   * The returned value should be an empty DOMstring (just for passing 

nit: here too.

::: dom/engineeringmode/nsEngineeringMode.h
@@ +11,5 @@
> +#define ENGINEERING_MODE_CID                            \
> +  { 0x68c5e530, 0x0c8a, 0x40c3,                         \
> +    { 0xa3, 0xc9, 0x80, 0x6a, 0xa7, 0xef, 0x03, 0x43 } }
> +
> +#define ENGINEERING_MODE_CONTRACTID "@mozilla.org/b2g/engineering-mode-impl;1"

we usually add that to the .idl file so it's available too c++ consumers just by including nsIEngineeringMode.h

::: dom/engineeringmode/nsEngineeringModeModule.cpp
@@ +73,5 @@
> +// The following line implements the one-and-only "NSGetModule" symbol
> +// for compatibility with mozilla 1.9.2. You should only use this
> +// if you need a binary which is backwards-compatible and if you use
> +// interfaces carefully across multiple versions.
> +//NS_IMPL_MOZILLA192_NSGETMODULE(&kEngineeringModeModule)

remove this part, it's super old!
Attachment #8485872 - Flags: review?(fabrice)
Comment on attachment 8481920 [details] [diff] [review]
bug-997564-activity-v2.patch

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

Im not super strong on this code, but it looks good to except for the first check in activitiesService.jsm which I dont completely understand.

::: dom/activities/src/ActivitiesService.jsm
@@ +213,5 @@
> +        "id": aMsg.id,
> +        "error": "SecurityError"
> +      });
> +      return;
> +    }

This check looks fine, but shouldn't you only be doing this check if the following is true? Maybe Im missing something but it looks like this would prevent _all_ web activities from being started unless the app was certified?

aMsg.options.name === 'internal-system-engineering-mode'

::: dom/activities/src/ActivityProxy.js
@@ +67,5 @@
> +      Services.DOMRequest.fireErrorAsync(this.activity, "SecurityError");
> +      Services.obs.notifyObservers(null, "Activity:Error", null);
> +      return;
> +    }
> +

Can we do this check in the parent instead? Or maybe this is just an additional check that isn't important for security but lets us return quickly in case the forbidden name is used? Am I understanding this correctly?

(IE there is another check in ActivitiesService.jsm which is the important security check)

Comment 73

5 years ago
Hi Evelyn,

We wanted to follow up on this issue.
We've applied attachment 8485856 [details] [diff] [review] & 8485872 but are still seeing "Can not get engineering mode implementation." error.

But we then moved the code out of nsEngineeringModeModule.cpp and into gecko\layout\build\nsLayoutModule.cpp
and this workaround seems to solve the component not registered problem.
Have any idea?

Our code base released from Qualcomm(FFOS 2.0 ES1).

Thanks.

This is for [Tako].
Attachment #8481920 - Flags: feedback?(ptheriault) → feedback+
Assignee

Comment 74

5 years ago
(In reply to Paul Theriault [:pauljt] from comment #72)
> Comment on attachment 8481920 [details] [diff] [review]
> bug-997564-activity-v2.patch
> 
> Review of attachment 8481920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Im not super strong on this code, but it looks good to except for the first
> check in activitiesService.jsm which I dont completely understand.
> 
> ::: dom/activities/src/ActivitiesService.jsm
> @@ +213,5 @@
> > +        "id": aMsg.id,
> > +        "error": "SecurityError"
> > +      });
> > +      return;
> > +    }
> 
> This check looks fine, but shouldn't you only be doing this check if the
> following is true? Maybe Im missing something but it looks like this would
> prevent _all_ web activities from being started unless the app was certified?
> 
> aMsg.options.name === 'internal-system-engineering-mode'
> 

Yep, you're right. I was missing this condition.

> ::: dom/activities/src/ActivityProxy.js
> @@ +67,5 @@
> > +      Services.DOMRequest.fireErrorAsync(this.activity, "SecurityError");
> > +      Services.obs.notifyObservers(null, "Activity:Error", null);
> > +      return;
> > +    }
> > +
> 
> Can we do this check in the parent instead? Or maybe this is just an
> additional check that isn't important for security but lets us return
> quickly in case the forbidden name is used? Am I understanding this
> correctly?
> 

We did another check in parent process (ActivitiesService.jsm) for only allowing certified app calling this activity, as you've seen above. So yes, the check in child process is for earlier return.

> (IE there is another check in ActivitiesService.jsm which is the important
> security check)

Yes. :)
Assignee

Comment 75

5 years ago
(In reply to JohnsonLin from comment #73)
> Hi Evelyn,
> 
> We wanted to follow up on this issue.
> We've applied attachment 8485856 [details] [diff] [review] & 8485872 but are
> still seeing "Can not get engineering mode implementation." error.
> 
> But we then moved the code out of nsEngineeringModeModule.cpp and into
> gecko\layout\build\nsLayoutModule.cpp
> and this workaround seems to solve the component not registered problem.
> Have any idea?
> 
> Our code base released from Qualcomm(FFOS 2.0 ES1).
> 
> Thanks.
> 
> This is for [Tako].

Hi, I don't know what happened but I will make a v2.0 version of my patches for you in these two days.
Assignee

Comment 76

5 years ago
Comment on attachment 8464620 [details] [review]
EngMode-Gaia-Implementation

Hi rik, test case added. Could you review again? thanks!
Attachment #8464620 - Flags: review?(anthony)
Assignee

Comment 77

5 years ago
Comment 70 addressed. I merged two manifest files into one. Thanks for the reviews!
Attachment #8485856 - Attachment is obsolete: true
Attachment #8489478 - Flags: review?(fabrice)
Comment on attachment 8489478 [details] [diff] [review]
Gecko-implementation-of-Engineering-mode-WebAPI-v4.patch

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

r=me with nits fixed.

::: dom/engineeringmode/EngineeringModeAPI.js
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const DEBUG = true;

nit: flip to false.

@@ +36,5 @@
> +                                         Ci.nsISupportsWeakReference,
> +                                         Ci.nsIObserver]),
> +
> +  init: function(aWindow) {
> +    this._window = aWindow;

No needed, it happens already in initDOMRequestHelper

@@ +38,5 @@
> +
> +  init: function(aWindow) {
> +    this._window = aWindow;
> +
> +    cpmm.sendAsyncMessage("EngineeringMode:Register", null);

move that after initDOMRequestHelper, to be sure we won't have races where the parent sends messages we haven't yet registered for.
Attachment #8489478 - Flags: review?(fabrice) → review+
Assignee

Comment 79

5 years ago
Carry over r+.
Attachment #8489478 - Attachment is obsolete: true
Attachment #8490772 - Flags: review+
Assignee

Comment 80

5 years ago
add reviewer in commit message. carry over r+.
Attachment #8490772 - Attachment is obsolete: true
Attachment #8490774 - Flags: review+
Assignee

Comment 81

5 years ago
Addressed Paul's comment 72. I would also like to have Fabrice review in this part. Thank you both.
Attachment #8481920 - Attachment is obsolete: true
Attachment #8490778 - Flags: review?(ptheriault)
Attachment #8490778 - Flags: review?(fabrice)
Comment on attachment 8464620 [details] [review]
EngMode-Gaia-Implementation

Thanks for the tests!
Attachment #8464620 - Flags: review?(anthony) → review+
Attachment #8490778 - Flags: review?(fabrice) → review+
Assignee

Comment 84

5 years ago
rebased against b2g-inbound. carry over r+.
Attachment #8490774 - Attachment is obsolete: true
Attachment #8492766 - Flags: review+
Assignee

Comment 85

5 years ago
rebased against b2g-inbound. carry over r+.
Attachment #8481921 - Attachment is obsolete: true
Attachment #8492767 - Flags: review+
Assignee

Comment 86

5 years ago
rebased against b2g-inbound. carry over r+.
Attachment #8490778 - Attachment is obsolete: true
Attachment #8490778 - Flags: review?(ptheriault)
Attachment #8492769 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8485872 - Attachment description: Refererenced-implementation-of-nsIEngineeringMode-v2.patch → [Do Not Checkin] Refererence-implementation-of-nsIEngineeringMode-v2.patch
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 87

5 years ago
Filed test case bug 1070734.
Hi Evelyn, seems part 1 needs dom peer review:

remote: WebIDL file dom/webidl/EngineeringMode.webidl altered in changeset 8bd7e88c77aa without DOM peer review

Could you take a look, thanks!
Assignee

Comment 89

5 years ago
Comment on attachment 8492766 [details] [diff] [review]
001-Gecko-implementation-of-Engineering-mode-WebAPI-v5.patch

Hi Jonas, Fabrice has r+ on implementation detail but I need a dom peer r+ on my dom changing part. Could you help me on this? Thank you.
Attachment #8492766 - Flags: review+ → review?(jonas)
Assignee

Updated

5 years ago
Keywords: checkin-needed

Comment 92

5 years ago
Hi Evelyn,

This is a follow up on Comment 75.
Just wanted to know that is there any suggestions you have that we could try on our side while you are working on the patch for v2.0?

Thanks.
Assignee

Comment 93

5 years ago
(In reply to FH Chen from comment #92)
> Hi Evelyn,
> 
> This is a follow up on Comment 75.
> Just wanted to know that is there any suggestions you have that we could try
> on our side while you are working on the patch for v2.0?
> 
> Thanks.

Hi Wayne, last time you told me our partner will migrate their engineering mode test to v2.1. Is it a confirmed information? Thanks. (also ni Francis to catch his attention)
Flags: needinfo?(wchang)
Flags: needinfo?(frlee)
Hi Evelyn,

The partner we spoke about is good to have this on 2.1, thanks!
Flags: needinfo?(wchang)
as comment 95, remove my ni.
Flags: needinfo?(frlee)
Assignee

Comment 97

5 years ago
back port to 2.0 per partner's request.
Assignee

Updated

5 years ago
Attachment #8499697 - Attachment description: [Do Not Checkin] v2.0-Refererence-implementation-of-nsIEngineeringMode-v2.patch → [Do Not Checkin] v2.0-Refererence-implementation-of-nsIEngineeringMode.patch

Updated

5 years ago
Whiteboard: [Tako_Blocker]
[Blocking Requested - why for this release]:
blocking-b2g: - → 2.1?
This is partner's blocker and they need this API available for enabling engineering mode which will be served for production testing.
set it to 2.1+ and ni Evelyn for processing uplift approval.
blocking-b2g: 2.1? → 2.1+
Flags: needinfo?(ehung)
Assignee

Comment 101

5 years ago
Comment on attachment 8492766 [details] [diff] [review]
001-Gecko-implementation-of-Engineering-mode-WebAPI-v5.patch

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 #): n/a
User impact if declined: partner needs it for their factory test.
Testing completed: manual test
Risk to taking this patch (and alternatives if risky): no
String or UUID changes made by this patch: I'm not sure what UUID means, but no string changes.
Flags: needinfo?(ehung)
Attachment #8492766 - Flags: approval-mozilla-b2g34?
Assignee

Comment 102

5 years ago
Comment on attachment 8492767 [details] [diff] [review]
0002-Gecko-Use-async-way-to-fire-an-error.patch

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 #): n/a
User impact if declined: partner needs it for their factory test.
Testing completed: manual test
Risk to taking this patch (and alternatives if risky): no
String or UUID changes made by this patch: I'm not sure what UUID means, but no string changes.
Attachment #8492767 - Flags: approval-mozilla-b2g34?
Assignee

Comment 103

5 years ago
Comment on attachment 8492769 [details] [diff] [review]
0003-Gecko-implementation-of-internal-system-engmode-activity-v3.patch

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 #): n/a
User impact if declined: partner needs it for their factory test.
Testing completed: manual test
Risk to taking this patch (and alternatives if risky): no
String or UUID changes made by this patch: I'm not sure what UUID means, but no string changes.
Attachment #8492769 - Flags: approval-mozilla-b2g34?
Attachment #8492766 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Attachment #8492767 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Attachment #8492769 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
(In reply to Evelyn Hung [:evelyn] from comment #101)
> Comment on attachment 8492766 [details] [diff] [review]
> 001-Gecko-implementation-of-Engineering-mode-WebAPI-v5.patch
> 
> 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 #): n/a
> User impact if declined: partner needs it for their factory test.
> Testing completed: manual test
> Risk to taking this patch (and alternatives if risky): no
> String or UUID changes made by this patch: I'm not sure what UUID means, but
> no string changes.

Your patch does not have nay IDl changes so all good here :)

Nevertheless, I'll give this my best shot on what it means : 

We sometimes make changes to interfaces that are publicly visible via IDL. These changes are necessary for our continued development, and we do not guarantee interface compatibility between releases. However, in order for binary addons to be compatible, they sometimes will check to verify if the binary version of a particular interface has changed using the IID. Any time an interface changes, the IID needs to be changed as
well.

Unfortunately, this is an easy thing to forget to do, and an even easier thing to miss during review. Even more unfortunate is that we don't know (currently) about these changes until after release (as that's when the plugins users are using break). WE have hit this in the past several times post release where we cause major crashes as a result of this incompatibility and in-order to avoid this and while the "hg" hook gets ready & we have automation in place here(Bug 477166) we request folks to answer this question, so uplifts until aurora have the IID bumped.
Assignee

Updated

4 years ago
Summary: launch engineering mode app → Engineering mode framework
You need to log in before you can comment on or make changes to this bug.