720614 - SMS android permissions being requested on trunk builds

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

what landed: http://mxr.mozilla.org/mozilla-central/source/configure.in#4637

isn't what got posted in the bug (and apparently not reviewed):
https://bugzilla.mozilla.org/attachment.cgi?id=584789&action=diff#a/configure.in_sec1

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Disable WebSMS by default.

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to Brad Lassey [:blassey] from comment #0)
> what landed: http://mxr.mozilla.org/mozilla-central/source/configure.in#4637
> 
> isn't what got posted in the bug (and apparently not reviewed):
> https://bugzilla.mozilla.org/attachment.cgi?id=584789&action=diff#a/
> configure.in_sec1

What landed is what I said I would do. The patch in the bug wasn't doing what I said (it was untested).

Anyhow, if having websms enabled in Nightly is an issue, we should disable that in mozconfigs.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 591340 [details] [diff] [review]
patch

r=blassey assuming you have tested this

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)
> (In reply to Brad Lassey [:blassey] from comment #0)
> > what landed: http://mxr.mozilla.org/mozilla-central/source/configure.in#4637
> > 
> > isn't what got posted in the bug (and apparently not reviewed):
> > https://bugzilla.mozilla.org/attachment.cgi?id=584789&action=diff#a/
> > configure.in_sec1
> 
> What landed is what I said I would do. The patch in the bug wasn't doing
> what I said (it was untested).
> 
> Anyhow, if having websms enabled in Nightly is an issue, we should disable
> that in mozconfigs.

Mounir you landed that patch with out any review at all, not just your customary insufficient review. On top of that, I was pretty explicit in discussing this that we don't want it enabled for any fennec builds, including trunk. There was no way that wasn't clear to you.

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to Brad Lassey [:blassey] from comment #4)
> Mounir you landed that patch with out any review at all, not just your
> customary insufficient review. On top of that, I was pretty explicit in
> discussing this that we don't want it enabled for any fennec builds,
> including trunk. There was no way that wasn't clear to you.

IIRC, I did ask a review for this patch saying that I wanted to have this enabled by default and being able to get disabled. khuey and you told me it was disabled by default and I must have attached another patch saying it will be ok like that. No one said nothing so I assumed it was. I guess there was a confusion with me thinking "it's now enabled by default" and you guys thinking "it's something I will be okay with". I did try to use the 'else' part of the autoconf macro but that doesn't seem to work.
I realize that it was actually disabled by default a few days before landing and I thought it wasn't worth re-asking a review given that I was changing the behavior for what I told it would be. I agree that I should ask a review for this change and I apologize for not doing this correctly.

However, if you want to have WebSMS disabled by default in all fennec builds, you should use --disable-websms-backend in the fennec mozconfigs. This patch is disabling this by default for all builds (including b2g builds). You can also add a condition in the configure.in if having this enabled in developer builds seems harmful.

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 591340 [details] [diff] [review]
patch

r- based on my previous comment: this is doing more than disabling WebSMS for Fennec.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 591340 [details] [diff] [review]
patch

Mounir, b2g should enable it in its mozconfig if they want it

Basically since you landed without review, either stop r-'ing this patch or back all of WebSMS out. Resetting your r- to a ? and I'll take your response as a decision.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

First of all, let's please leave personal feelings at the door, and interact like professionals.  Grievances should be aired through appropriate channels.

All "desktop" platforms are unaffected by the changes here, so we don't need to discuss them.

I think we all agree on these goals
 - SMS should be enabled for b2g
 - SMS should be disabled for firefox-android builds shipped to users (pending strategic discussion to take place elsewhere)

We want this SMS-android code for non-firefox uses of gecko-android, and likely firefox-android pending discussion.  Because of that, it would be nice to
 - leave SMS enabled for developer builds, so it doesn't bitrot

What comprises "developer builds" is less clear.  It's not really fair to bounce tryserver/m-i builds for breaking a feature that's not shipping yet.

Also, the SMS code is pretty well isolated, so the risk of bitrot seems fairly low to me.  Mounir, do you have more specific concerns about bitrottage?

To achieve all these goals, the options are
 1. Globally disable SMS by default, specifically enable it for b2g and local devs.
 2. Globally enable SMS by default, specifically disable it for all android mozconfigs in buildbot.

I lean towards (1) just because it's simpler.  Any objections?

Comment 9

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)

> To achieve all these goals, the options are
>  1. Globally disable SMS by default, specifically enable it for b2g and
> local devs.
>  2. Globally enable SMS by default, specifically disable it for all android
> mozconfigs in buildbot.
> 
> I lean towards (1) just because it's simpler.  Any objections?

#1 seems ok to me as well. Unless I am missing something (I am not the smartest build stuff guy) Brubeck's patch would make #1 work, except we would want to add MOZ_WEBSMS_BACKEND=1 to b2g/confvars.sh, right?

http://mxr.mozilla.org/mozilla-central/source/b2g/confvars.sh

That would enable SMS for b2g and the b2g devs, I think.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm not intimately familiar with confvars.sh either, but that or changing this mozconfig option to --enable-websms and using that in b2g would be fine too.  Your idea seems simpler, so I like it better.

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 591340 [details] [diff] [review]
patch

I guess one r+ is enough.

Comment 12

[Matt Brubeck (:mbrubeck)]

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

Comment 13

[Matt Brubeck (:mbrubeck)]

Backed out because of Android build bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/547cea3b54fd

/builds/slave/m-in-andrd-xul/build/obj-firefox/config/nsinstall -D classes
"/tools/jdk6/bin/javac" -target 1.5 -source 1.5 -classpath /tools/android-sdk-r13/platforms/android-13/android.jar -bootclasspath /tools/android-sdk-r13/platforms/android-13/android.jar -encoding UTF8 -g  -d classes  /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoApp.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoAppShell.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoConnectivityReceiver.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoEvent.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoSurfaceView.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoInputConnection.java /builds/slave/m-in-andrd-xul/build/embedding/android/AlertNotification.java /builds/slave/m-in-andrd-xul/build/embedding/android/SurfaceInfo.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoBatteryManager.java /builds/slave/m-in-andrd-xul/build/embedding/android/VideoPlayer.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoNetworkManager.java App.java Restarter.java NotificationHandler.java LauncherShortcuts.java SmsManager.java  CrashReporter.java R.java
SmsManager.java:41: cannot find symbol
symbol  : class GeckoSmsManager
location: package org.mozilla.gecko
import org.mozilla.gecko.GeckoSmsManager;
                        ^
SmsManager.java:51: cannot find symbol
symbol  : class GeckoSmsManager
location: class org.mozilla.gecko.SmsManager
      sInstance = new GeckoSmsManager();
                      ^
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
2 errors

Comment 14

[Matt Brubeck (:mbrubeck)]

Actually, this built successfully some of the time after the original landing; maybe it just requires a clobber.

Comment 15

[Matt Brubeck (:mbrubeck)]

I've clobbered Android on inbound and retriggered the failed build.  If it goes green, I will re-push with a notice that clobbering is required.

Comment 16

[Matt Brubeck (:mbrubeck)]

Re-landed after clobbering Android on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a57afa0226

Comment 17

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/87a57afa0226

Comment 18

[Mark Goodwin [:mgoodwin]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> I think we all agree on these goals
>  - SMS should be enabled for b2g
>  - SMS should be disabled for firefox-android builds shipped to users
> (pending strategic discussion to take place elsewhere)

I notice that the current Firefox (10.0.0.2) and Beta (11.0) versions in the Android Marketplace both request permission to send and receive SMS. From looking at some marketplace reviews, it seems some other people have noticed this too...

Has this discussion taken place yet? Is there a reason this should be enabled for firefox-android currently?

Comment 19

[Aaron Train [:aaronmt]]

(In reply to Mark Goodwin [:mgoodwin] from comment #18)
> I notice that the current Firefox (10.0.0.2) and Beta (11.0) versions in the
> Android Marketplace both request permission to send and receive SMS. From
> looking at some marketplace reviews, it seems some other people have noticed
> this too...
> 
> Has this discussion taken place yet? Is there a reason this should be
> enabled for firefox-android currently?

it is only 11.0 Beta 3 and it was a mistake: bug 728327