Hook up WebSMS to new WebAPI permission manager

RESOLVED FIXED in mozilla17

Status

()

Core
General
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: vicamo)

Tracking

(Blocks: 1 bug)

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:S])

Attachments

(4 attachments, 9 obsolete attachments)

8.03 KB, patch
Details | Diff | Splinter Review
23.58 KB, text/plain
Details
8.34 KB, patch
Details | Diff | Splinter Review
9.29 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
WebSMS's permissions are currently a hack.  But once we have the proper permission manager, we can fix this.
No longer depends on: 707625
Depends on: 707625
Blocks: 774716
blocking-basecamp: --- → +

Updated

5 years ago
Priority: -- → P1
(Assignee)

Updated

5 years ago
Assignee: nobody → vyang
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 652045 [details] [diff] [review]
Part 1: DOM implementation
(Assignee)

Comment 2

5 years ago
Created attachment 652057 [details] [diff] [review]
Part 1: DOM implementation : V2

remove perf "dom.sms.whitelist" from modules/libpref/src/init/all.js as well.
Attachment #652045 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Created attachment 652059 [details] [diff] [review]
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?
Attachment #652059 - Flags: feedback?(philipp)
(Assignee)

Comment 4

5 years ago
Created attachment 652071 [details] [diff] [review]
Part 2: fix test scripts : V2

* Fix marionette test cases as well
* mochitest is in https://tbpl.mozilla.org/?tree=Try&rev=d7efa8ccec8b
Attachment #652059 - Attachment is obsolete: true
Attachment #652059 - Flags: feedback?(philipp)
(Assignee)

Updated

5 years ago
Whiteboard: [LOE:S]
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 760199
(Assignee)

Comment 6

5 years ago
Created attachment 652336 [details] [diff] [review]
Part 1: DOM implementation : V3

Update patch for hg
Attachment #652057 - Attachment is obsolete: true
Attachment #652336 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #652336 - Flags: review? → review?(justin.lebar+bug)
(Assignee)

Comment 7

5 years ago
Created attachment 652338 [details] [diff] [review]
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.
Attachment #652071 - Attachment is obsolete: true
Attachment #652338 - Flags: review?(philipp)
(Assignee)

Comment 8

5 years ago
trying https://tbpl.mozilla.org/?tree=Try&rev=b87daa7d2820
(Reporter)

Comment 9

5 years ago
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.
Attachment #652336 - Flags: review?(justin.lebar+bug) → review+
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
Attachment #652338 - Flags: review?(philipp) → review+
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 652728 [details] [diff] [review]
Part 1: DOM implementation : V4

address Justin's review in comment #9
Attachment #652336 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Created attachment 652729 [details] [diff] [review]
Part 2: fix test scripts : V4

Address Philipp's review opinions in comment #10
Attachment #652338 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 652730 [details] [diff] [review]
[DO NOT SUBMIT] debug
(Assignee)

Comment 15

5 years ago
Created attachment 652731 [details]
Emulator Mochitest adb logcat
(Assignee)

Comment 16

5 years ago
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.
(Reporter)

Comment 17

5 years ago
> 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.
(Assignee)

Comment 18

5 years ago
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?
(Assignee)

Comment 19

5 years ago
Created attachment 654070 [details] [diff] [review]
Part 1: DOM implementation : V5

rebase
Attachment #652728 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 654072 [details] [diff] [review]
Part 2: fix test scripts : V5

add "unittest.expectedFailure" annotation back
Attachment #652729 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
try again: https://tbpl.mozilla.org/?tree=Try&rev=2c8ac9a7cc45
(Assignee)

Updated

5 years ago
Blocks: 784617
(Assignee)

Comment 22

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
Created attachment 654131 [details] [diff] [review]
Part 2: fix test scripts : V6

only checkSmsEnabled in Android build
Attachment #654072 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
try yet again: https://tbpl.mozilla.org/?tree=Try&rev=021e574fe29d
(Assignee)

Comment 25

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b87b0846c913
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5ca56e66668
https://hg.mozilla.org/mozilla-central/rev/b87b0846c913
https://hg.mozilla.org/mozilla-central/rev/c5ca56e66668
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
This bug added new NSPR types.  Please avoid doing that!  (See bug 579517 for more info).  Thanks!
(Assignee)

Updated

5 years ago
Blocks: 785867
You need to log in before you can comment on or make changes to this bug.