920831 - MozInputMethod's init should only return undefined

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Andrew McCreight [:mccr8]]

As seen in ConstructJSImplementation, the init method for JS-implemented WebIDL should only return undefined, because unlike the XPIDL-based JS-implementation, we ignore the return value of init.  The init for MozInputMethod returns null if it doesn't have permission.

I'm not sure exactly what the right approach is to detect the permission, maybe using a Func attribute like PushManager?
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/PushManager.webidl#7

(Also, we should consider crashing when an init() method doesn't return undefined, if people return other values in the case when permission fails...)

Comment 1

[Andrew McCreight [:mccr8]]

(gwagner hit this assertion)

Comment 2

[Andrew McCreight [:mccr8]]

I guess this should be a security bug, though we haven't shipped it.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Andrew McCreight [:mccr8] from comment #0)
> As seen in ConstructJSImplementation, the init method for JS-implemented
> WebIDL should only return undefined, because unlike the XPIDL-based
> JS-implementation, we ignore the return value of init.  The init for
> MozInputMethod returns null if it doesn't have permission.
> 
> I'm not sure exactly what the right approach is to detect the permission,
> maybe using a Func attribute like PushManager?
> http://mxr.mozilla.org/mozilla-central/source/dom/webidl/PushManager.webidl#7
> 

Yes, see bug 884897.

Also filed bug 920840.

Comment 4

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

 Andrew, thank you. It seems easy to fix and I'm taking it;-)

Comment 5

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

Attachment: 0001-navigator.patch

The patch uses a static method Navigator::HasInputMethod to detect whether to expose MozInputMethod to navigator.

Boris, I guess you're the right person to review the patch, but if you don't have time, feel free to pass on.

Comment 6

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

Attachment: bug-920831-v1.patch

fix nits.

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

In one of my work for 906096 I just returned undefined if there was no permission instead of null. Couldn't that work?

Comment 8

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

Yes, but partially works. We also need to hide mozInputMethod attribute if there is no permission. Returning undefined won't hide mozInputMethod.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Jan Jongboom [:janjongboom] from comment #7)
> In one of my work for 906096 I just returned undefined if there was no
> permission instead of null. Couldn't that work?

Undefined is not used to indicate success/error. Rather think of init() as returning 'void'. Which is why we need the C++ function to check for permission.

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

Thanks :nsm and :yxl for explaining.

Comment 11

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

Boris, thank you.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/96974bd168a5

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G test_interfaces.html orange.
https://hg.mozilla.org/integration/b2g-inbound/rev/0c156ee959d0

https://tbpl.mozilla.org/php/getParsedLog.php?id=28424422&tree=B2g-Inbound

Comment 14

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

Attachment: Fix test interfaces.

The permission check prevents test_interfaces.html from accessing MozInputMethod without 'keyboard' permission.

The patch modifies the test_interfaces.html to bypass MozInputMethod.

Comment 15

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

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Folded the two patches together and rebased on top of bug 906096.
https://hg.mozilla.org/integration/b2g-inbound/rev/51a41b139305

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Yeah, that'll teach me. My rebasing was wrong. Needs someone who knows webidl to do the right thing.
https://hg.mozilla.org/integration/b2g-inbound/rev/5e3d6f3b13a5

Comment 18

[Boris Zbarsky [:bzbarsky]]

The Pref bit in the idl needs to go away.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/bfde80bc6826

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Needs more than that. I'll leave it to yxl to sort out. Backed out.
https://hg.mozilla.org/integration/b2g-inbound/rev/ed1bed4fbaab

Comment 21

[Andrew McCreight [:mccr8]]

This is the error:
In file included from UnifiedBindings11.cpp:12:
./InputMethodBinding.cpp:1909:21: error: no member named 'HasInputMethodSupport' in 'mozilla::dom::Navigator'
  return Navigator::HasInputMethodSupport(cx, obj);
         ~~~~~~~~~~~^
1 error generated.

yxl: The bustage is in non-B2G builds.  HasInputMethodSupport needs to be defined in non-B2G builds and return false.  Or something like that.

Comment 22

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

Ryan & Andrew, thanks for your help. Bug 906096enables MozInputMethod on non-B2G build and I should remove the macro '#ifdef MOZ_B2G'. 

I'll provide a rebased patch.

Comment 23

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

Attachment: rebased.

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

Comment 24

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

Attachment: gecko patch v2

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6768887622d4

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6768887622d4

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/23aa697cf2a9

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Is this b2g only? What are the security implications of this bug?

This didn't go through sec-approval so I'm a bit in the dark on it. See https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 29

[Andrew McCreight [:mccr8]]

Looking over this some more, I think the flags are wrong.  The regressing bug, bug 899073, only landed in Firefox 26, and it was fixed everywhere, so we never shipped it.

Comment 30

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

(In reply to Al Billings [:abillings] from comment #28)
> Is this b2g only? What are the security implications of this bug?
It is b2g only. Without fixing this bug, the b2g app can use MozInputMethod

Comment 31

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

(In reply to Al Billings [:abillings] from comment #28)
> Is this b2g only? What are the security implications of this bug?
It is b2g only. Without fixing this bug, the b2g app can use MozInputMethod API even if its permission check fails.