707659 - Hook up WebSMS to new WebAPI permission manager

Status: RESOLVED FIXED

(Core :: General, defect, P1)

Description

[Justin Lebar (not reading bugmail)]

WebSMS's permissions are currently a hack.  But once we have the proper permission manager, we can fix this.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: DOM implementation

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: DOM implementation : V2

remove perf "dom.sms.whitelist" from modules/libpref/src/init/all.js as well.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: fix test scripts : WIP

test_sms_basics.html restores original whitelist preference setting, but there doesn't seem to be a corresponding API in permission manager. There is no 'isGranted' or 'tryAskPermission' or a similar one. And, is it important at all?

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: fix test scripts : V2

* Fix marionette test cases as well
* mochitest is in https://tbpl.mozilla.org/?tree=Try&rev=d7efa8ccec8b

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 652071 [details] [diff] [review]
Part 2: fix test scripts : V2

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

::: dom/sms/tests/marionette/test_incoming.js
@@ +31,5 @@
>    ok(message.timestamp instanceof Date);
> +  // Comparing timestamp of two emulator instances might be meaningless.
> +  // Just try to ensure the timestamp attribute was assigned with an
> +  // appropriate value.
> +  ok(Math.abs(message.timestamp.getTime() - now) < 5 * 60 * 1000);

This part was previously open as bug 760199. This test is not running with two emulator instances. I'll remove it. See also bug 760199 comment #5.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: DOM implementation : V3

Update patch for hg

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: fix test scripts : V3

* update patch for hg
* remove changes in test_incoming.js that belongs to bug 760199
* ignore restoring sms permission in test_sms_basics.html and explicitly remove it instead.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

trying https://tbpl.mozilla.org/?tree=Try&rev=b87daa7d2820

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 652336 [details] [diff] [review]
Part 1: DOM implementation : V3

> +/* static */already_AddRefed<SmsManager>
> +SmsManager::CheckPermissionAndCreateInstance(nsPIDOMWindow* aWindow)

Hey, I've seen this code before...

We may want to abstract this a bit sometime, but this is fine for now.  :)

Please add the check for the dom.sms.enabled pref into CheckPermissionAndCreateInstance, though.

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 652338 [details] [diff] [review]
Part 2: fix test scripts : V3

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

Thanks, Vicamo!

::: testing/marionette/client/marionette/marionette_test.py
@@ +67,3 @@
>  Components.utils.import("resource://gre/modules/Services.jsm");
> +let pm = Components.classes["@mozilla.org/permissionmanager;1"]
> +                   .getService(Components.interfaces.nsIPermissionManager);

Services.perms

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Justin Lebar [:jlebar] from comment #9)
> Please add the check for the dom.sms.enabled pref into
> CheckPermissionAndCreateInstance, though.

Wow, that's funny. The test script doesn't work at all :(

In dom/sms/tests/test_sms_basics.html, function checkSmsEnabled():

> // WebSms is disabled on all platforms except Android for the moment.
> if (navigator.appVersion.indexOf("Android") == -1) {
>   checkSmsDisabled();
>   return;
> }

The appVersion string got in B2G emulator is "5.0 ()", so it checks whether SMS is disabled throughout the test script.

With above lines removed, there are still errors in the test result. This time, `Preferences::GetBool("dom.sms.enabled", defaultSmsPermission)` always returns true and breaks several test cases. Then I found:

> nsresult
> Preferences::GetBool(const char* aPref, bool* aResult);

Well, the function prototype is totally different at all.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: DOM implementation : V4

address Justin's review in comment #9

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: fix test scripts : V4

Address Philipp's review opinions in comment #10

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [DO NOT SUBMIT] debug

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Emulator Mochitest adb logcat

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 652731 [details]
Emulator Mochitest adb logcat

> ...
>I/GeckoDump(  424): TEST-PASS | unknown test url | SmsCursor should be prefixed
>I/GeckoDump(  424): TEST-PASS | unknown test url | SmsCursor should be prefixed
>I/Navigator(  393): PREF_SetBoolPref(dom.sms.enabled, false, false)
>I/Navigator(  437): PREF_SetBoolPref(dom.sms.enabled, false, true)

Why are there two `PREF_SetBoolPref` called?

>I/Navigator(  393): PREF_GetBoolPref(dom.sms.enabled, 0xbedb3898, false)
>I/GeckoDump(  424): TEST-PASS | unknown test url | dom.sms.enabled - false should equal false

TEST-PASS because SpecialPowers gets correct modified value.

>I/GeckoDump(  424): TEST-PASS | unknown test url | Checking [false, false, disabled]
>I/GeckoDump(  424): TEST-PASS | unknown test url | navigator.mozSms should exist
>I/Navigator(  424): Navigator::GetMozSms - mSmsManager is null
>I/Navigator(  424): Navigator::GetMozSms - window got
>I/Navigator(  424): PREF_GetBoolPref(dom.sms.enabled, 0xbeac631f, false)
>I/Navigator(  424): SmsManager::CheckPermissionAndCreateInstance - enabled

In native code, we get an unmodified dom.sms.enabled value and may fail following tests. Why?

>I/Navigator(  424): SmsManager::CheckPermissionAndCreateInstance - document got
>I/Navigator(  424): SmsManager::CheckPermissionAndCreateInstance - principal got
>I/Navigator(  424): SmsManager::CheckPermissionAndCreateInstance - permMgr got
>I/Navigator(  424): SmsManager::CheckPermissionAndCreateInstance - permission removed
>I/GeckoDump(  424): TEST-PASS | unknown test url | navigator.mozSms should return null - null should equal null

TEST-PASS here, but actually we get null SmsManager for no access, not SMS is disabled as expected.

Comment 17

[Justin Lebar (not reading bugmail)]

> Wow, that's funny. The test script doesn't work at all :(

Awesome.

> `Preferences::GetBool("dom.sms.enabled", defaultSmsPermission)` always returns true and breaks 
> several test cases. Then I found:
>
> nsresult
> Preferences::GetBool(const char* aPref, bool* aResult);
>
> Well, the function prototype is totally different at all.

That function has an override, though:

>  static bool GetBool(const char* aPref, bool aDefault = false)

See modules/libpref/public/Preferences.h.

> Why are there two `PREF_SetBoolPref` called?

I don't know.  You could stick a breakpoint there and see, if you're curious.  (gdb) call DumpJSStack() will show you the JS caller, if there is one on the stack.

> In native code, we get an unmodified dom.sms.enabled value and may fail following tests. Why?

I don't know what you mean here.  I'm happy to try to answer if you can clarify.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

After a weekend, I got another problem that Mochitest runs test cases in OOP and it always hangs in:

> PContentChild::SendPSmsConstructor(PSmsChild* actor) {
>   ...
>   bool __sendok = (mChannel).Send(__msg);
>   ...
> }

I think I'd better give up for now to get this issue solved. I'll upload another revision for part 2 to keep "unittest.expectedFailure" mark. Any suggestions?

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: DOM implementation : V5

rebase

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: fix test scripts : V5

add "unittest.expectedFailure" annotation back

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

try again: https://tbpl.mozilla.org/?tree=Try&rev=2c8ac9a7cc45

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21)
> try again: https://tbpl.mozilla.org/?tree=Try&rev=2c8ac9a7cc45

Still breaks mochitests of platforms other than Android :(
I opened a new bug 784617 for this and will update patches later.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: fix test scripts : V6

only checkSmsEnabled in Android build

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

try yet again: https://tbpl.mozilla.org/?tree=Try&rev=021e574fe29d

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b87b0846c913
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5ca56e66668

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b87b0846c913
https://hg.mozilla.org/mozilla-central/rev/c5ca56e66668

Comment 27

[(no longer active)]

This bug added new NSPR types.  Please avoid doing that!  (See bug 579517 for more info).  Thanks!