967218 - (Synth APK) - User visible debugging enabled notification shown

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect)

Description

[Aaron Train [:aaronmt]]

After launching AccuWeather on my device today I noticed a 'debugging enabled' notification 962607. I am told in #mobile that APK's generated are currently marked as debug and thus is the reason. Figure we can keep this bug open until we're off the development server.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Actually we should fix this bug on the development server, as that server is user-facing, and we shouldn't be serving debug APKs to end users.

(We do want to serve debug APKs to developers, of course.  And it's confusing that we call this a "development server", because it suggests that it's the server we use to serve APKs to developers.  Whereas it's actually just the server we use to develop the server.)

Comment 2

[Austin King [:ozten]]

dapk.net doesn't have a release and review deployment.

This will be fixed when we point Nightly at production's release environment. Does that work?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

No, I don't think we should wait until we point users at production's release environment, since we want Nightly users to test the user-facing experience implemented by synthetic APKs.

(We may also want to set up a server that serves debug APKs, so webapp developers can test the developer-facing experience; and one for reviewers, as appropriate; but end-user testing is the immediate goal.)

Comment 4

[Austin King [:ozten]]

I didn't want to have the public dev environment setup like a production deployment because
1) It's a bunch of sysadmin work for me
2) It's a continous maintenance / deployment tax

Currently dapk.net signs apks the way the prototype did, we're blocked on a stage/production deployment of the APK Signer service before we can serve APKs from a reviewer as well as a release channel.

IT is building out stage/production for APK Factory as well as APK Signer.

I suggest production, because of the following thoughts:
1) We can point Nightly at either stage or production since both will have both a reviewer and release endpoint
2) We should probably point Nightly / Aurora at production, since the system will stabilize at some point and we'd want to test changes in stage before pushing to production.

We can switch things up, this is my reasoning.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: does string comparison

(In reply to Austin King [:ozten] from comment #4)
> I didn't want to have the public dev environment setup like a production
> deployment because
> 1) It's a bunch of sysadmin work for me
> 2) It's a continous maintenance / deployment tax

I don't want that either!


> Currently dapk.net signs apks the way the prototype did, we're blocked on a
> stage/production deployment of the APK Signer service before we can serve
> APKs from a reviewer as well as a release channel.

Sure, I understand.  What I'm suggesting is that in the meantime we serve APKs from a release channel, as our most important audience of testers right now is users (Nightly users, MWC demoers, etc.).


In any case, I dug into this further, and it turns out that dapk.net is actually generating release APKs, since its buildWithAnt function calls `ant release` to generate them:

https://github.com/mozilla/apk-factory-service/blob/e7a9b23460b342d8b932489fcc6334a96bc5a58e/lib/apk_project_builder.js#L207-L209

Thus this bug is client-side: GeckoApp.handleMessage sets mCurrentResponse to the string "false" rather than boolean false if the app is not debuggable, which eventually becomes the return value of sendMessageToJava({ type: "NativeApp:IsDebuggable" }), where it gets mistakenly treated as a boolean and thus true.

mCurrentResponse is of type String, and making it a variant is a bigger chore.  And it's going away anyway once wesj finishes standing up asynchronous messaging.  So let's fix this with a simple string comparison for the time being.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8371139 [details] [diff] [review]
patch v1: does string comparison

Good catch!

Comment 7

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b7825f65dd

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7b7825f65dd

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8371139 [details] [diff] [review]
patch v1: does string comparison

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 962607.
User impact if declined: 
  A remote debugger connection will be started for all webapps; users will see
  a notification for each app they launch.
Testing completed (on m-c, etc.):
  This has been on Central for a week.
Risk to taking this patch (and alternatives if risky): 
  This is a simple, low-risk patch that corrects an obvious datatype bug in a
  conditional.
String or IDL/UUID changes made by this patch:
  None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80708dff29bc