974770 - Get rid of dom.mozInputMethod.testing in test scripts

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It looks like we don't need |dom.mozInputMethod.testing| for mochitests. We should be able to get all the permissions we need with SpecialPowers.

I have a WIP patch ready.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: WIP

I am able to get dom/inputmethod/mochitests to pass locally. However browser-element test was enabled and did not work with or without my modification here, even through bug 906096 have long been fixed.

I am now trying to fix it altogether.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: WIP

Upload the right file.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It looks like the |frames[0].setInputMethodActive(true);| DOM request does not fire onsuccess nor onerror.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: WIP

The DOM request didn't fire because there wasn't any inputcontext issued to the input method app. I have to put the input field in another mozbrowser frame for the input context to be issued correctly.

The test now fails at the point here input method #0 did not being turned inactive in time, and sends a second "#0" the the input. I will try to figure out the timing issue carefully.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Damn, |count| should start from 1 ....

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1

This patch re-enables setInputMethodActive tests and remove dom.mozInputMethod.testing.

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

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8378991 [details] [diff] [review]
Patch v1

Canceling review because try orange. I can reproduce the same error (but not timeout) locally.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v2

BrowserElementParent throw because activeInputFrame is already a dead object. I put that part of code into a try catch block.

It seems that the oop tests will sometimes timeout. Should I simply comment it out (and file a bug)?

https://tbpl.mozilla.org/?tree=Try&rev=185c74740505

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8379566 [details] [diff] [review]
Patch v2

Cancel review, the test failed at ICS Emulator Opt m-6.

Xulei, let me know what you think before I continue working on this.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It looks like the tests will always timed out on Android and Emulator will failed to get permission (inproc or oop).

Comment 11

[Yuan Xulei [:yxl] (Leave from May 18)]

Tim. I'm sorry and I just noticed this bug.
I failed to set "input" permission to mozbrowser when OOPed. That's why I used a pref to bypass permission check. It seems we cannot remove this pref :(

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

I'll review the patch tommorrow.

Comment 13

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8379566 [details] [diff] [review]
Patch v2

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

::: dom/browser-element/BrowserElementParent.jsm
@@ +716,5 @@
>      // Deactivate the old input method if needed.
>      if (activeInputFrame && isActive) {
> +      try {
> +        // This will throw if activeInputFrame is already a dead object,
> +        // if so, we whould simply set it to null directly.

Can we use |Cu.isDeadWrapper(activeInputFrame)| to determine if activeInputFrame is dead to avoid try-catch?

::: dom/browser-element/mochitest/browserElement_SetInputMethodActive.js
@@ +42,5 @@
> +  }], createFrames);
> +}
> +
> +var frames = [];
> +var inputFrame;

Add |g| prefix for these global variables, such as gFrames and gInputFrame.

@@ +47,5 @@
> +
> +function createFrames() {
> +  // Create two input method iframes.
> +  let loadendCount = 0;
> +  var countLoadend = function() {

Use |let| if possible.

@@ +92,2 @@
>    for (let i = 0; i < 2; i++) {
> +    var frame = frames[i] = document.createElement('iframe');

It is the same here. We prefer |let|.

@@ +107,2 @@
>  
> +  var req0 = frames[0].setInputMethodActive(true);

let

@@ +115,5 @@
> +  };
> +}
> +
> +var timerId = null;
> +let count = 0;

Add |g| prefix for global variables.

@@ +120,5 @@
> +
> +function next(msg) {
> +  count++;
> +  var wrappedMsg = SpecialPowers.wrap(msg);
> +  var value = wrappedMsg.data;

let

@@ +163,2 @@
>          tearDown();
> +      }, 1000);

When we test on try servers, sometimes the system is quite slow. It is likely that we get another input message before setInputMethodActive(false) succeeds. To avoid such case, we may make |inputFrame| generate messages less frequently or check |setInputMethodActive(false)| result after req3.onsuccess.

Comment 14

[Yuan Xulei [:yxl] (Leave from May 18)]

Hi Tim, I really like the fix you made for the mozbrowser method - setInputMethodActive. Could you land the fix first? May be with a new bug.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Yuan Xulei [:yxl] from comment #14)
> Hi Tim, I really like the fix you made for the mozbrowser method -
> setInputMethodActive. Could you land the fix first? May be with a new bug.

Sure.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I've filed bug 981997 will rebase Attachment #8379566 [details] [diff] and the submit the dom/browser-element part as the new patch in that bug.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Have no time to deal with this bug for a week.

There are two things left to do on this bug:

1. rebase the patch after bug 981997. Remove the part landed there.
2. figure out why we still need the perf for the permission in Emulator. It may be a timing issue since Emulator is so slow.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #17)
> 2. figure out why we still need the perf for the permission in Emulator. It
> may be a timing issue since Emulator is so slow.

I was too busy to investigate on this....

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

So according to Vicamo, I should use pushPermissions(inPermissions, callback) and run tests after the callback returns.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: bug974770-v2.patch

This is the entirely newly made patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5dbea13d0b2

I think the problem with Emulator previously was around permission propagation to content process -- and I hope the above try push can prove that it's already fixed on m-c.

This patch removes all reference to the pref and also the WebIDL [Func], and replace it with a [CheckAnyPermissions] descriptor.

https://dxr.mozilla.org/mozilla-central/search?q=dom.mozInputMethod.testing&redirect=false&case=true&limit=61&offset=0

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This blocks WebIDL clean-up because I would also like to clean up permission checks of methods in that bug.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: bug974770-v2.patch

Should be removed from MozKeyboard.js too.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dc284d3b2ba

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #20)
> I think the problem with Emulator previously was around permission
> propagation to content process -- and I hope the above try push can prove
> that it's already fixed on m-c.

To clarify, I was thinking about bug 1129315.

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #23)
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #20)
> > I think the problem with Emulator previously was around permission
> > propagation to content process -- and I hope the above try push can prove
> > that it's already fixed on m-c.
> 
> To clarify, I was thinking about bug 1129315.

And it's still there and not fixed.

https://treeherder.mozilla.org/logviewer.html#?job_id=10710771&repo=try

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I decided to disable those two tests instead.

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: bug974770-v2.patch

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #24)
> https://treeherder.mozilla.org/logviewer.html#?job_id=10710771&repo=try

Actually, accordingly to the log, only 

dom/browser-element/mochitest/test_browserElement_inproc_SetInputMethodActive.html
dom/browser-element/mochitest/test_browserElement_oop_SetInputMethodActive.html

failed and other SpecialPowers.pushPermissions() calls in dom/inputmethod/mochitests/ worked as expected. The difference here is that there is a little timeout after the pushPermissions() call.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #25)
> I decided to disable those two tests instead.

So, instead, I would add a timeout there too and see if it works.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7da05b87329

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #26)
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #24)
> > https://treeherder.mozilla.org/logviewer.html#?job_id=10710771&repo=try
> 
> Actually, accordingly to the log, only 
> 
> dom/browser-element/mochitest/
> test_browserElement_inproc_SetInputMethodActive.html
> dom/browser-element/mochitest/test_browserElement_oop_SetInputMethodActive.
> html
> 
> failed and other SpecialPowers.pushPermissions() calls in
> dom/inputmethod/mochitests/ worked as expected. The difference here is that
> there is a little timeout after the pushPermissions() call.
> 
> (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to
> queue) from comment #25)
> > I decided to disable those two tests instead.
> 
> So, instead, I would add a timeout there too and see if it works.

I managed to fixed my local emulator build again, and a the setTimeout does fix it... so this try run will fail too. Let's figure out what to do later.

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Actually dom/inputmethod/ tests are not run on emulators because of bug 983015.

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: bug974770-v2.patch

Test disabled.

Comment 30

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c92c13c781b0

Comment 31

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8652268 [details] [diff] [review]
bug974770-v2.patch

Masayuki san,

Could you review this? This is a small clean-up on the permission check we are using. I have to disable one test in Emulator because the permission cannot be dispatched in the content process -- already have a bug filed on that.

Thanks!

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8652268 [details] [diff] [review]
bug974770-v2.patch

Hmm, I'm not familiar with the security model of b2g applications. Therefore, I'm really afraid to review this. Aren't there better people to review this in b2g staff?

Comment 33

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8652268 [details] [diff] [review]
bug974770-v2.patch

:kanru, are you eligible of giving review on this? Thanks.

I know a few DOM peers that should be able to review b2g-related permissions, but all of them have long review queue, as suggested reviewers have shown.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Why you remove dom.mozInputMethod.enabled check for the interface?

Comment 35

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Olli Pettay [:smaug] (high review load) from comment #34)
> Why you remove dom.mozInputMethod.enabled check for the interface?

Ouch, did I?

Comment 36

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: bug974770-v2.patch

Patch updated to add the Pref= flag. Thanks :smaug for reminder.

Comment 37

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8652807 [details] [diff] [review]
bug974770-v2.patch

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

::: dom/browser-element/mochitest/mochitest-oop.ini
@@ +85,5 @@
>  [test_browserElement_oop_SelectionStateBlur.html]
>  skip-if = (toolkit == 'gonk') # Disabled on b2g due to bug 1097419
>  [test_browserElement_oop_SendEvent.html]
>  [test_browserElement_oop_SetInputMethodActive.html]
> +skip-if = (os == "android") || (toolkit == 'gonk') #b2g only bug 1198163

s/b2g only/Disable on b2g/

::: dom/browser-element/mochitest/mochitest.ini
@@ +208,5 @@
>  skip-if = (toolkit == 'gonk') # Disabled on b2g due to bug 1097419
>  [test_browserElement_inproc_SendEvent.html]
>  # The setInputMethodActive() tests will timed out on Android
>  [test_browserElement_inproc_SetInputMethodActive.html]
> +skip-if = (os == "android") || (toolkit == 'gonk') #b2g only bug 1198163

ditto

Comment 38

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Kan-Ru Chen [:kanru] from comment #37)
> Comment on attachment 8652807 [details] [diff] [review]
> bug974770-v2.patch
> 
> Review of attachment 8652807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/mochitest/mochitest-oop.ini
> @@ +85,5 @@
> >  [test_browserElement_oop_SelectionStateBlur.html]
> >  skip-if = (toolkit == 'gonk') # Disabled on b2g due to bug 1097419
> >  [test_browserElement_oop_SendEvent.html]
> >  [test_browserElement_oop_SetInputMethodActive.html]
> > +skip-if = (os == "android") || (toolkit == 'gonk') #b2g only bug 1198163
> 
> s/b2g only/Disable on b2g/

Disabled on b2g emulator is even better.

Comment 39

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch to commit

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/989bd5496610

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/989bd5496610