Keyboard app fails permission check for mozSettings because it has localID from system app

VERIFIED FIXED

Status

VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: gwagner, Assigned: vingtetun)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Real fix for bug 801045
(Reporter)

Updated

6 years ago
Blocks: 801045
blocking-basecamp: --- → ?

Updated

6 years ago
blocking-basecamp: ? → +

Comment 1

6 years ago
Gregor, whats the diagnosis here?
(Reporter)

Comment 2

6 years ago
During the installation phase (first run) I see that we put the correct permission (settings) into the permission DB with the correct origin and appId for the keyboard app.

During keyboard initialization we call navigator.mozSettings. There we check the permission for the window.document.nodePrincipal. The principal has the correct URI for the keyboard app but nodePrincipal.appId is the appID from the system app.
Fabrice was talking about an appID transformation during install. Investigating...
This could be related to general brokenness in the way we propagate appId to child processes. jlebar?
Oops, /me finally CCs jlebar.
I assume this means that any attempt by the keyboard to query settings will fail in current builds.  

This would mean that the keyboard is ignoring all settings: language, preferred layout, word suggestions, key click sounds, vibration.

Gregor: in case it isn't already clear:

We would eventually like to allow the user to be able to install third-party keyboard apps.  We're not there yet. The keyboard app is really just part of the system app, loaded with the rest of the system app at boot time (see apps/system/js/keyboard_manager.js) But it pretends to be a regular app in its own directory because that's the direction we want to move in.

If the appID transformation fix you guys are talking about is a general purpose thing that gets us closer to the goal of enabling installable keyboards, and is also easy to do, then that would be a great way to fix this.

But for v1, another approach would be to just admit that the keyboard is part of the system app and move it there.  I'm cc'ing Tim because he probably understands this better than I do.
(In reply to ben turner [:bent] from comment #3)
> This could be related to general brokenness in the way we propagate appId to
> child processes. jlebar?

This sounds like things are behaving in Gecko exactly as intended.
(Reporter)

Comment 7

6 years ago
(In reply to David Flanagan [:djf] from comment #5)
> I assume this means that any attempt by the keyboard to query settings will
> fail in current builds.  
> 

This only shows up if we install gaia with "make reset-gaia" or "make production".
It works if we do a full flash via ./flash.sh
(Reporter)

Comment 8

6 years ago
Vingtetun pointed out that the keyboard is loaded in the same iframe as the system app. So this should be fixed in gaia:

vingtetun: gwagner: i believe we should make the keyboard a mozbrowser iframe that use the keyboard manifest then. this will create some issue with keyboard resizing thought (it uses to postMessage it's parent)
vingtetun: but that would be for good
The keyboard also uses parent.postMessage() to ask the system app to show it and hide it, so that communication channel is critical.

But postMessage() works cross origin, doesn't it? So if the keyboard can obtain a reference to the system app's window (i.e. a replacement for parent) it should still be able to send message with postMessage().

I don't know what the security issues are with exposing the system app's window to other apps.

Perhaps the navigator.mozKeyboard API could be extended somehow to provide the communication channel, or to define methods for opening, closing and resizing the keyboard.  Right now, mozKeyboard just has onfocuschange and sendKey.  Maybe it also needs showKeyboard(url_of_keyboard_app) which would cause the window manager to show the specified app as a keyboard.

Seems like a kind of big architectural change to make at this stage, though.

Repeating comment #5: we could also consider just moving the keyboard into the system app on the assumption that installable keyboard apps are not a v1 goal.  This would also allow us to keep the mozKeyboard API private, since it really isn't ready for public use.
> So if the keyboard can obtain a reference to the system app's window (i.e. a replacement for parent) 
> it should still be able to send message with postMessage().

One main purpose of iframe mozbrowser is that you can't get a reference to your parent.

Even if you can figure out a way around this, I don't think you should.  If you want to do the equivalent of postMessage into and out of an <iframe mozbrowser>, we need to modify the browser API.

Comment 11

6 years ago
The keyboard is 7MB of RAM or more. I would much prefer not adding that to the system app.
(In reply to Andreas Gal :gal from comment #11)
> The keyboard is 7MB of RAM or more. I would much prefer not adding that to
> the system app.

OTOH if you push it out into a separate process, the overhead increases to 7MB plus our process overhead, which is more than 5MB.

It seems to me to be extremely late in the game to suggest that it's time to move the keyboard out of process.
Can we simply add the ability to show and hide the keyboard to the keyboard API?

Comment 14

6 years ago
That memory is reclaimable though. The system app is not.

Comment 15

6 years ago
What does #13 solve?
(In reply to Andreas Gal :gal from comment #14)
> That memory is reclaimable though. The system app is not.

The keyboard costs 7mb of memory in the system app even when it's not shown?  That seems like a serious bug somewhere...

Comment 17

6 years ago
We load a dictionary that is around 5-7mb depending on language. I am not sure there is an easy way to unload but we can try.
It's too late in the game to move keyboard OOP.  There are too many unsolved problems raised by doing so.
Andreas, Chris: I don't even really think that this is about in process vs. out of process.

The keyboard is not really an app.  It is just part of the system app. When Gaia starts up, the system app loads the keyboard, even though the keyboard is packaged separately, it is run in an iframe that is part of the system app.  Here's the relevant system app code that loads the keyboard: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/keyboard_manager.js#L20

Making the keyboard into a real app (whether in process or OOP) is feature work not bug fixing, and I don't think we should try to do it for v1.

On the other hand, moving all the keyboard code from apps/keyboard to apps/system would probably be a simple change and would presumably get rid of this permissions problem. 

Andreas: I'm having to refactor a lot of the keyboard code sort of brutally, so it shouldn't be too hard to defer loading the dictionary until it is actually needed and then discard it (maybe 30 seconds after the keyboard is hidden?)  I'll file a bug and cc you on that.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> It's too late in the game to move keyboard OOP.  There are too many unsolved
> problems raised by doing so.

Keyboard app should be an OOP app after v1 though.

(In reply to Justin Lebar [:jlebar] from comment #16)
> (In reply to Andreas Gal :gal from comment #14)
> > That memory is reclaimable though. The system app is not.
> 
> The keyboard costs 7mb of memory in the system app even when it's not shown?
> That seems like a serious bug somewhere...

If we were killing OOP keyboard app when it's not shown, I am afraid that user would have to wait for keyboard to show to start typing. Keyboard API (bug 737110) would need to be redesigned too since it currently only give out information of the focused input on the oninputfocus callback -- keyboard app loads after the input focus will not be given the information.
Taking for now. I will have some fun with the keyboard and I would like to fix this in the same row.
Assignee: nobody → 21
Vivien: I'm in the middle of a major refactor of the keyboard app for the various predictive text bugs.  In particular, I've already rewritten all of the settings-related code.  So don't have too much fun with the keyboard!
Created attachment 672859 [details] [diff] [review]
gaia patch

David this Gaia patch does not change too much of the keyboard code but make it a mozbrowser iframe and solve the permission issue. (it is still not OOP).
Attachment #672859 - Flags: review?(dflanagan)
Created attachment 672864 [details] [diff] [review]
Gecko patch

Chris this patch comes with the Gaia patch that contains a event dispatcher when the keyboard is up. So events are caught before they hit content and are redispatched if needed. No click are lost. (And this will fix bug 796452).

The main issue for now is with the interaction with the browser. When the keyboard is up, if the user touch the scrollable area then a mousedown is fired by this code and the focus change. This can result into the keyboard beeing dismissed. This basically introduce the same issue as bug 766066 for the browser. With some changes to what events the dispatcher sent this can be possible to workaround that but I feel like this is out of the scope of this patch.
Attachment #672864 - Flags: review?(jones.chris.g)
Comment on attachment 672859 [details] [diff] [review]
gaia patch

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

I like the mozbrowserlocationchange communication channel.  That's really nice.

I'm nervous about your approach to forwarding events to the underlying app through a transparent overlay. Why is that necessary?

I've just read through #796452...  Is the only reason that we need this whole transparent overlay thing to allow the light blue alternate key popups to float over content?  Couldn't we just redesign the keyboard a bit so that the popups stayed within the bounds of the keyboard?  For the top row of keys, they could popup right or left instead of above.  And anyway, with the work I'm doing on word suggestions, I'm hoping to have that enabled by default, which gives us an extra 30 pixels or so above the top row of keys.

Also, if this whole event handling scheme is just for the transient popups, couldn't we just show the overlay when needed and then hide the entire overlay when the popup goes away?  The user won't be interacting with the keyboard to create one of those popups and at the same time trying to interact with the content below the overlay right?

I guess I just don't understand what bugs it was that lead to this issue, and it just seems like there ought to be a simpler way.

So I'm going to withhold r+ until:

1) you convince me that we can't tweak the keyboard or hide and show the transparent overlay instead of doing this event dispatch hack.

2) you convince me that it is not necessary to dispatch touchmove events along with touchstart and touchend.

3) you explain what the changes in render.js are for.

::: apps/browser/js/browser.js
@@ +1651,5 @@
>  
> +// Browser is in-process and the events dispatched by the parent
> +// needs to be captured in order to not throw a new event in the
> +// parent and enter into an infinite loop.
> +window.addEventListener('mousedown', function(e) {

I don't really understand what this is doing, but it is unforunate that it is needed.  Is browser really the only in-process app? I thought that contacts or sms was still in process, too.

Please modify the comment to explain that this is related to the keyboard, or maybe even reference this bug in the comment.

::: apps/keyboard/js/render.js
@@ +401,5 @@
>      var changeScale, scale;
>  
>      // Font size recalc
> +    changeScale = window.innerWidth / 32;
> +    document.documentElement.style.fontSize = changeScale + 'px';

This seems completely unrelated to the other changes. It looks like it completely changes the definition of 1 rem for the keyboard in landscape mode.  Does the keyboard still look okay when rotated?

@@ +406,4 @@
>  
> +    var ime = document.getElementById('keyboard');
> +    ime.classList.remove('landscape');
> +    ime.classList.add('portrait');

these two classes appear to be unused in keyboard.css, and since you're never adding landscape or removing portrait, I think you can just remove these three lines.

::: apps/system/index.html
@@ +600,3 @@
>        <!-- keyboard -->
> +      <div id="keyboard-frame" data-z-index-level="keyboard-frame"></div>
> +      <div id="keyboard-event-dispatcher" data-z-index-level="keyboard-frame"></div>

Could you swap the order of these two frames, since the dispatcher is going to be positioned above the keyboard?

::: apps/system/js/keyboard_manager.js
@@ +66,5 @@
> +    touchstart = false;
> +
> +    forwardTouchEventToContent(e);
> +  }, true);
> +

You're not forwarding touch move events. Will that break touch-and-swipe gestures for selecting a region of text?

@@ +107,5 @@
> +  }
> +
> +  // Listen for mozbrowserlocationchange of keyboard iframe.
> +  var a = document.createElement('a');
> +  keyboard.addEventListener('mozbrowserlocationchange', function(e) {

clever communication channel!
Comment on attachment 672859 [details] [diff] [review]
gaia patch

r- for now, but maybe you just need to explain it all to me more carefully.  (Or find another reviewer who understands the underlying issues more thoroughly :-)
Attachment #672859 - Flags: review?(dflanagan) → review-
I'm trying to get to this review, but finding it difficult do so when my brain is fully functioning :(.  Sorry, hope to do this tomorrow.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #27)
> I'm trying to get to this review, but finding it difficult do so when my
> brain is fully functioning :(.  Sorry, hope to do this tomorrow.

I will resolve this issue without this. Let's remove that from your pile for now.
(In reply to David Flanagan [:djf] from comment #26)
> Comment on attachment 672859 [details] [diff] [review]
> gaia patch
> 
> r- for now, but maybe you just need to explain it all to me more carefully. 
> (Or find another reviewer who understands the underlying issues more
> thoroughly :-)

I have likely tried to shoot 2 things with the same patch. I will resolve the issue of this bug with the new communication channel only and let the event dispatch mechanism for an other bug. Thanks!
Attachment #672864 - Attachment is obsolete: true
Attachment #672864 - Flags: review?(jones.chris.g)
Created attachment 673880 [details] [diff] [review]
Patch v2

David I believe this patch will make you happier.
Attachment #672859 - Attachment is obsolete: true
Attachment #673880 - Flags: review?(dflanagan)
Comment on attachment 673880 [details] [diff] [review]
Patch v2

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

I don't have time for a full review right now, but I don't think the code can work with this typo in it.

::: apps/system/js/keyboard_manager.js
@@ +11,3 @@
>    }
>  
> +  function generateKeyboard(contaimer, keyboardURL, manifestURL) {

s/contaimer/container/
(In reply to David Flanagan [:djf] from comment #31)
> Comment on attachment 673880 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 673880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't have time for a full review right now, but I don't think the code
> can work with this typo in it.
> 
> ::: apps/system/js/keyboard_manager.js
> @@ +11,3 @@
> >    }
> >  
> > +  function generateKeyboard(contaimer, keyboardURL, manifestURL) {
> 
> s/contaimer/container/

It does. Thanksfully to the js scope magic and the fact that I initialize a |var container| variable a bit later.
Comment on attachment 673880 [details] [diff] [review]
Patch v2

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

I've tried out this patch and it works for me.

Please fix the typo.  Everything else is a nit, and shouldn't prevent you from landing this.

r=djf.

::: apps/system/js/keyboard_manager.js
@@ +11,3 @@
>    }
>  
> +  function generateKeyboard(contaimer, keyboardURL, manifestURL) {

s/contaimer/container/

@@ +24,5 @@
> +  // Generate a <iframe mozbrowser> containing the keyboard.
> +  var container = document.getElementById('keyboard-frame');
> +  var keyboardURL = getKeyboardURL();
> +  var manifestURL = keyboardURL.replace('index.html', 'manifest.webapp');
> +  var keyboard = generateKeyboard(container, keyboardURL, manifestURL);

I think I'd take the index.html out of getKeyboardURL() and then add index.html and manifest.webapp to the two urls here, rather than adding it in and then having to replace it.  But that's a nit.

@@ +29,2 @@
>  
> +  // The overlay will display part of the keyboard that are below the

s/below/above/ ?

@@ +36,2 @@
>  
> +  var a = document.createElement('a');

nit: I'd change this variable name to 'urlparser' or something.

@@ +50,5 @@
>  
> +        var updateHeight = function() {
> +          container.removeEventListener('transitionend', updateHeight);
> +          overlay.style.height = height + 'px';
> +          container.classList.add('visible');

Is the visible class actually needed?

@@ +63,4 @@
>  
> +        if (container.classList.contains('hide')) {
> +          container.classList.remove('hide');
> +          container.addEventListener('transitionend', updateHeight);

I don't understand why the transition listener is needed here? What would happen if you just set the overlay height as the same time you removed the 'hide' class?

I always find it very hard to understand the desired effect when reading code like this.  A comment would be helpful.

@@ +69,2 @@
>  
> +        updateHeight();

I think this would be better in an else clause without the return above. It confused me this way.

::: apps/system/style/system/keyboard.css
@@ +10,5 @@
> +}
> +
> +#keyboard-frame.visible {
> +  visibility: visible;
> +  pointer-events: auto;

Aren't both of these styles the default values?  If you've removed the 'hide' class, does adding the 'visible' class actually change anything?

@@ +37,5 @@
> +  border: 0;
> +  background: -moz-element(#keyboard-frame);
> +  background-position: top center;
> +  pointer-events: none;
> +}

I think I've finally figured out what this is doing...

The overlay div is transparent to events, but the -moz-element allows it to display any popups that the keyboard displays outside of the main keyboard area, right?

It would be nice to have a comment explaining that.  But I know you plan to replace it with something better, so that is not necessary.
Attachment #673880 - Flags: review?(dflanagan) → review+
Duplicate of this bug: 805007
https://github.com/mozilla-b2g/gaia/pull/5997
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Vivien, congratulations for the `mozbrowserlocationchange` solution, (it is a pity the event is not standard, grrr). The solution reminds me about the Cost Control driver: generating the iframe on the fly.

It is a very smart solution and you can encode some basic JSON in the hash to be decoded by the parent. Anyway I don't know if this will work when `remote=true` for the iframe (Will the event be dispatched even with `remote=false`. I suppose). Anyway, until `remote=false`, the keyboard remains not OOP. Doesn't it?
> Anyway I don't know if this will work when `remote=true` for the iframe (Will the event 
> be dispatched even with `remote=false`. I suppose).

mozbrowserlocationchange will work for remote=true.
Another remark. Consider these commentaries:

(In reply to David Flanagan [:djf] from comment #5)
> But for v1, another approach would be to just admit that the keyboard is
> part of the system app and move it there.

(In reply to David Flanagan [:djf] from comment #19)
> Andreas: I'm having to refactor a lot of the keyboard code sort of brutally,
> so it shouldn't be too hard to defer loading the dictionary until it is
> actually needed and then discard it (maybe 30 seconds after the keyboard is
> hidden?)  I'll file a bug and cc you on that.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #20)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> > It's too late in the game to move keyboard OOP.  There are too many unsolved
> > problems raised by doing so.
> 
> Keyboard app should be an OOP app after v1 though.

This is important. See later.
 
> (In reply to Justin Lebar [:jlebar] from comment #16)
> > (In reply to Andreas Gal :gal from comment #14)
> > > That memory is reclaimable though. The system app is not.
> > 
> > The keyboard costs 7mb of memory in the system app even when it's not shown?
> > That seems like a serious bug somewhere...
> 
> If we were killing OOP keyboard app when it's not shown, I am afraid that
> user would have to wait for keyboard to show to start typing. Keyboard API
> (bug 737110) would need to be redesigned too since it currently only give
> out information of the focused input on the oninputfocus callback --
> keyboard app loads after the input focus will not be given the information.

Ok. All this reminds me to the Cost Control application. Vivien and I are discussing about how to move OOP the CC application loosing as less functionality as possible.

Problems I'm planned to discuss tomorrow are the same as those pointed above: if B2G decides to kill the Keyboard application or the Cost Control application, the system will spend a lot of time (delay that the user will notice) opening each application again. And bot, Keyboard and Cost Control widget are application very likely to be used.
(In reply to Salvador de la Puente González [:salva] from comment #36)
> Anyway, until `remote=false`, the keyboard remains not OOP. Doesn't it?

Mistake here, I mean `remote=true`.

Thank you :jlebar
With all this, I want to say that there are applications that are required very often, so we should think about what is better: response time for first usage (every time after the application is closed) or memory usage (taking in count all the open/close overhead).

Maybe B2G could schedule them to close in the last term or maybe they should never been closed at all.

I think it deserves a deep though.
Component: DOM: Apps → Gaia
Product: Core → Boot2Gecko
Duplicate of this bug: 806387

Comment 42

6 years ago
I'm not sure if this the bug where the world icon was re-added (since bug 806387 is a dupe of this; I think it is) but I want to point out that the world icon is back but there is an issue.

I only have English selected. I would assume that I would not have multiple languages to rotate through with the world icon but instead I have *all* languages to rotate through.

Can we please only show the languages that I have selected on my settings?
Should I file a separate bug?

Comment 43

6 years ago
Verified on 11/14/2012 nightly build using the steps from 
https://bugzilla.mozilla.org/show_bug.cgi?id=806387.   "World" icon now appears, with all languages selected in settings (although there are other issues now with intl kb).  New bugs on the way!
Status: RESOLVED → VERIFIED

Updated

6 years ago
Component: Gaia → Gaia::Keyboard
You need to log in before you can comment on or make changes to this bug.